Remove usages of 'Foo' from our codebase. #21224

CyrusNajmabadi · 2017-07-31T22:28:46Z

It doesn't pass policheck.

CyrusNajmabadi · 2017-07-31T22:31:19Z

Tagging @jaredpar . I preserved casing as that's very significant for many usages of these variables. i.e. if we had "fOo" it was changed to "gOo".

I expect some failures after the first wave. Specifically, for things like completion or matching where we'll have to change what is typed to match. i.e. we'll likely have to change an "f" that was typed in a test to a "g".

jamesqo · 2017-07-31T22:56:41Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -1063,7 +1063,7 @@ public IAliasSymbol GetAliasInfo(IdentifierNameSyntax nameSyntax, CancellationTo
            if (!CanGetSemanticInfo(nameSyntax))
                return null;

-            SymbolInfo info = GetSymbolInfoWorker(nameSyntax, SymbolInfoOptions.PreferTypeToConstructors | SymbolInfoOptions.PreserveAliases, cancellationToken);
+            SymbolInfo info = GetSymbolInfoWorker(nameSyntax, SymbolIngoOptions.PreferTypeToConstructors | SymbolIngoOptions.PreserveAliases, cancellationToken);


This seems unintentional

sharwell

For all practical purposes, I'm 👎 on this change unless both of the following conditions are met:

Test cases which changed are individually reviewed to ensure the new test preserves the original intent of the test.
The new name in each case is more meaningful in the context of the test.

Outside of these conditions, I do not believe it is possible for the incremental benefit to outweigh the increased change of a future regression going uncaught.

CyrusNajmabadi · 2017-08-01T01:59:01Z

The new name in each case is more meaningful in the context of the test.

I don't see any reason why the new name needs to be any more meaningful. It can be equally as meaningless as the current name. The benefit here is that we are now no longer being flagged by policheck. And it makes it easier for new code to checked for not adding new violations.

jaredpar · 2017-08-01T16:24:16Z

// TODO: Consider reducing the table memory gootprint.

This edit should be reverted

Refers to: src/Compilers/Core/Portable/Desktop/DesktopAssemblyIdentityComparer.Fx.cs:14 in 1044d3a. [](commit_id = 1044d3a6778418390327d105f2f8355b94fe464f, deletion_comment = False)

jaredpar · 2017-08-01T16:24:42Z

    /// when the resulting reference becomes unreachable and GC collects it. To decrease memory gootprint of the reference and/or manage

This edit should be reverted.

Refers to: src/Compilers/Core/Portable/MetadataReference/MetadataReference.cs:182 in 1044d3a. [](commit_id = 1044d3a6778418390327d105f2f8355b94fe464f, deletion_comment = False)

jaredpar · 2017-08-01T16:24:49Z

    /// when the resulting reference becomes unreachable and GC collects it. To decrease memory gootprint of the reference and/or manage

This edit should be reverted.

Refers to: src/Compilers/Core/Portable/MetadataReference/MetadataReference.cs:220 in 1044d3a. [](commit_id = 1044d3a6778418390327d105f2f8355b94fe464f, deletion_comment = False)

jaredpar · 2017-08-01T16:42:23Z

@CyrusNajmabadi for any future edits here please push commits vs. a rebase. Will help with the review as it's a giant change. We can squash before merging.

CyrusNajmabadi · 2017-08-01T16:42:27Z

Changes made. Also, updated tool to not match these in the future. Words that start with 'foo' must be followed by a non-letter, or 'b', or a capital letter. 'fool, food, foot' will no longer be caught. I also went and audited all the gool's, goot's and 'good's' to transform them back.

jaredpar · 2017-08-01T16:57:22Z

I have reviewed all of the compiler changes and they look good to me. Will need sign off from other parts of the code base though.

CyrusNajmabadi · 2017-08-01T18:36:36Z

for any future edits here please push commits vs. a rebase.

Will do!

CyrusNajmabadi · 2017-08-01T18:36:50Z

@dotnet/roslyn-ide can you take a look?

dpoeschl

Did a pretty thorough spot check and only found the "ifoo" thing and the file name thing (and the BOM thing)

dpoeschl · 2017-08-01T19:52:40Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs

@@ -1,4 +1,4 @@
-using Microsoft.CodeAnalysis.CSharp.Symbols;
+using Microsoft.CodeAnalysis.CSharp.Symbols;


The first line of every file (with/without copyright header) appears changed. What's up with that?

@dpoeschl yeah it's the BOM. we should fix that as @sharwell just did a change to add the BOM, this appears to be undoing that.

Guess codeflow hides this difference which is why I missed it.

dpoeschl · 2017-08-01T19:56:12Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs

-            IFoo ifoo = foo;
+            Goo goo = new Goo();
+            goo.M();
+            IGoo ifoo = goo;


"ifoo" seems suspicious

dpoeschl · 2017-08-01T20:09:47Z

src/Workspaces/CoreTestUtilities/TestFiles/OtherStuff_Foo.cs



-public class Foo
+public class Goo


The file name is OtherStuff_Foo.cs

Discussed with stakeholders. We're moving ahead with this.

CyrusNajmabadi · 2017-08-01T20:36:35Z

Bom bom bom bom Bombara Ann

CyrusNajmabadi · 2017-08-01T20:36:44Z

BOMs have been ordered.

CyrusNajmabadi · 2017-08-01T20:38:47Z

@jaredpar Are you good now that BOMs have returned?

CyrusNajmabadi · 2017-08-01T22:53:31Z

retest windows_debug_vs-integration_prtest please

…sion-expression-rewrite * dotnet/features/ioperation: (31 commits) Fix lambda EnC (dotnet#21291) Use 15.3 Preview 7 for VSI CI machines Add cancellation between each document. Remove unused private method (dotnet#21268) Work around SDK 1.1 issue around netcoreapp2.0 Update Language Feature Status.md Const warnings shouldn't cause emit error (dotnet#20729) Fix the symbol test Choose test config based on TargetFramework Standardize on the portable ThrowingTraceListener Enforce using UnitTestPortable Correct the order of parameters to Version Clean up RunCsc/Vbc.cmd a tiny bit Remove usages of 'Foo' from our codebase. (dotnet#21224) Make RunCsc/Vbc scripts respect DOTNET_HOST_PATH Fix typo ignoring exceptions about not running on main thread Updating VisualStudioSymbolSearchProgressService to consume new API Updating visual studio references Accept pre-rel version number ...

dnfclas added the cla-already-signed label Jul 31, 2017

CyrusNajmabadi force-pushed the foobar branch 3 times, most recently from 218d526 to 8f07791 Compare July 31, 2017 22:45

jamesqo reviewed Jul 31, 2017

View reviewed changes

CyrusNajmabadi force-pushed the foobar branch from 8d18d73 to 50658a7 Compare August 1, 2017 00:15

CyrusNajmabadi closed this Aug 1, 2017

CyrusNajmabadi force-pushed the foobar branch from 2be9c1b to e7869bd Compare August 1, 2017 00:34

CyrusNajmabadi reopened this Aug 1, 2017

dnfclas added the cla-already-signed label Aug 1, 2017

CyrusNajmabadi closed this Aug 1, 2017

CyrusNajmabadi force-pushed the foobar branch from 244ec40 to e7869bd Compare August 1, 2017 00:51

CyrusNajmabadi reopened this Aug 1, 2017

dnfclas added the cla-already-signed label Aug 1, 2017

CyrusNajmabadi force-pushed the foobar branch 2 times, most recently from 36ecf71 to 4edc5b8 Compare August 1, 2017 01:19

sharwell previously requested changes Aug 1, 2017

View reviewed changes

CyrusNajmabadi force-pushed the foobar branch 6 times, most recently from 998bb8a to 6d15d8f Compare August 1, 2017 01:48

CyrusNajmabadi force-pushed the foobar branch 5 times, most recently from f4ee1c0 to 90f5fc3 Compare August 1, 2017 02:47

CyrusNajmabadi force-pushed the foobar branch 2 times, most recently from 23872bb to 6bdf1c7 Compare August 1, 2017 16:38

Rename Foo to Goo

0e9239a

CyrusNajmabadi force-pushed the foobar branch from 6bdf1c7 to 0e9239a Compare August 1, 2017 16:38

Rename ifoo to igoo

af2dba9

dpoeschl approved these changes Aug 1, 2017

View reviewed changes

CyrusNajmabadi force-pushed the foobar branch from b7efe25 to af2dba9 Compare August 1, 2017 20:53

Add back BOMs

8259de3

CyrusNajmabadi force-pushed the foobar branch from c51c194 to d8cf563 Compare August 1, 2017 21:28

Merge remote-tracking branch 'upstream/master' into foobar

4b60176

CyrusNajmabadi force-pushed the foobar branch from d8cf563 to 4b60176 Compare August 1, 2017 21:33

CyrusNajmabadi merged commit 94ab292 into dotnet:master Aug 1, 2017

CyrusNajmabadi deleted the foobar branch August 1, 2017 23:44

sharwell mentioned this pull request Sep 18, 2018

Fix invalid cast converting Markdown code blocks DotNetAnalyzers/DocumentationAnalyzers#43

Merged

Youssef1313 mentioned this pull request Sep 24, 2020

Support EditorBrowsable in completion for unimported types #47771

Merged

Youssef1313 mentioned this pull request Nov 2, 2022

Simplify multiple casts in 'add explicit cast'. #65182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usages of 'Foo' from our codebase. #21224

Remove usages of 'Foo' from our codebase. #21224

CyrusNajmabadi commented Jul 31, 2017

CyrusNajmabadi commented Jul 31, 2017

jamesqo Jul 31, 2017

sharwell left a comment •

edited

Loading

CyrusNajmabadi commented Aug 1, 2017

jaredpar commented Aug 1, 2017

jaredpar commented Aug 1, 2017

jaredpar commented Aug 1, 2017

jaredpar commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

jaredpar commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

dpoeschl left a comment •

edited

Loading

dpoeschl Aug 1, 2017

dpoeschl Aug 1, 2017

jaredpar Aug 1, 2017

dpoeschl Aug 1, 2017

dpoeschl Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017 •

edited by dpoeschl

Loading

CyrusNajmabadi commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017 •

edited

Loading

CyrusNajmabadi commented Aug 1, 2017

		@@ -1,4 +1,4 @@
		using Microsoft.CodeAnalysis.CSharp.Symbols;
		using Microsoft.CodeAnalysis.CSharp.Symbols;

Remove usages of 'Foo' from our codebase. #21224

Remove usages of 'Foo' from our codebase. #21224

Conversation

CyrusNajmabadi commented Jul 31, 2017

CyrusNajmabadi commented Jul 31, 2017

jamesqo Jul 31, 2017

Choose a reason for hiding this comment

sharwell left a comment • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 1, 2017

jaredpar commented Aug 1, 2017

jaredpar commented Aug 1, 2017

jaredpar commented Aug 1, 2017

jaredpar commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

jaredpar commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

dpoeschl left a comment • edited Loading

Choose a reason for hiding this comment

dpoeschl Aug 1, 2017

Choose a reason for hiding this comment

dpoeschl Aug 1, 2017

Choose a reason for hiding this comment

jaredpar Aug 1, 2017

Choose a reason for hiding this comment

dpoeschl Aug 1, 2017

Choose a reason for hiding this comment

dpoeschl Aug 1, 2017

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 1, 2017 • edited by dpoeschl Loading

CyrusNajmabadi commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017 • edited Loading

CyrusNajmabadi commented Aug 1, 2017

sharwell left a comment •

edited

Loading

dpoeschl left a comment •

edited

Loading

CyrusNajmabadi commented Aug 1, 2017 •

edited by dpoeschl

Loading

CyrusNajmabadi commented Aug 1, 2017 •

edited

Loading