Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove ILiteralExpression.Text #21426

Merged
merged 11 commits into from Aug 22, 2017
Merged

Remove ILiteralExpression.Text #21426

merged 11 commits into from Aug 22, 2017

Conversation

333fred
Copy link
Member

@333fred 333fred commented Aug 10, 2017

Fixes #21300.

CyrusNajmabadi
CyrusNajmabadi reviewed Aug 10, 2017
View reviewed changes
src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs Outdated
object value;
if (operation.ConstantValue.HasValue && ((value = operation.ConstantValue.Value) == null ? "null" : value.ToString()) == operation.Text)
{
LogString($" (Text: {operation.Text})");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the change easier, you coudl just write out operation.Syntax.ToString here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already fixed it everywhere, and I'd prefer not to have a synthetic property getting printed.

@333fred
Fixed more test text changes.
45a2a89
@333fred
Merge remote-tracking branch 'dotnet/features/ioperation' into remove…
6335890 
…-literal-text

* dotnet/features/ioperation:
  Fix NamedArgumentInParameterOrderWithDefaultValue test for new IOperation output.
  EnC and EE cleanup (dotnet#21226)
  Fix crash when encountering a parenthesized expression when converting an if to a switch.
  this makes OOM to crash OOP process. this won't crash VS, instead it will show info bar and notify users to close VS and re-open.
  Turn on ref assemblies in all projects (dotnet#21346)
  Re-baseline some emit tests
  Move MakeFrames logic into Analysis
  Use langver=latest for scripting (dotnet#21331)
  Enable skipped tests and fix them (dotnet#21335)
  Replace project reference with linked file
  removed left out from deleted esent code.
@333fred
Copy link
Member Author

333fred commented Aug 10, 2017

@dotnet-bot retest windows_determinism_prtest please

@333fred
Copy link
Member Author

333fred commented Aug 10, 2017 •
edited

FYI @dotnet/roslyn-infrastructure test failure was #21435

@333fred
Copy link
Member Author

333fred commented Aug 11, 2017

Ping @dotnet/roslyn-compiler @AlekseyTs can I get a couple of reviews please?

@333fred
Merge remote-tracking branch 'dotnet/features/ioperation' into remove…
628d2ef 
…-literal-text

* dotnet/features/ioperation:
  Recommend 'case' keyword after a pattern-case-clause.
  Trim the length of the banner that we show for outlining down a bit.
  Disallow converting overrides of system.object methods.
  Use 'default' over 'default(T)' (dotnet#21387)
  Fix issue where Metadata as source wasn't properly handling Xml doc files with LF newlines.
  Warn if user replaces a method that overrides a metadata method with a property.
  Fix tests on cs-CZ culture (dotnet#21171)
  Correct comment about parameterized property
  If an exception is null, log accordingly
  Delete workaround for a long-fixed editor bug
  Typo
@cston
Copy link
Member

cston commented Aug 11, 2017

LGTM

@333fred
Copy link
Member Author

333fred commented Aug 14, 2017

Ping @dotnet/roslyn-compiler for a second review please.

jaredpar
jaredpar reviewed Aug 14, 2017
View reviewed changes
src/Compilers/Core/Portable/Operations/ILiteralExpression.cs
/// <summary>
/// Textual representation of the literal.
/// </summary>
string Text { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point in having an interface with no members? In general I'm suspicious of marker interfaces.

Copy link
Member Author

@333fred 333fred Aug 14, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, it's not empty. It has all the standard members of IOperation. Text was basically just providing a shortcut on literalExpression.Syntax.ToString(), which we don't feel worth having.

@333fred
Merge remote-tracking branch 'dotnet/features/ioperation' into remove…
d3e3511 
…-literal-text

* dotnet/features/ioperation:
  Update CSharpReplaceMethodWithPropertyService.cs
  Add VB side of fix.
  Remove unneeded function.
  Improve trivia preservation when converting methods into a property.
  VB side of do-not-simplify-nameof if it changes semantics.
  Do not simplify to an alias in a nameof if it changes the value of hte nameof.
  Include System.Runtime.Serialization.Primitives and System.Security.Cryptography.Csp in PortableFacades CoreXT package. (dotnet#21438)
  Address one more refactoring feedback
  Address PR feedback
  Fix possible race conditions in TestExtensionErrorHandler
  Fix expected test results to properly consider trivia
  Improve messages when tests fail due to expected text
  Default to considering trivia during testing
  Remove RemoveUnneededReferences from LamdaRewriter (dotnet#21367)
  Enable embedding sources to Windows PDBs (dotnet#21391)
  re-enabled assert we have disabled
  Do not insert Microsoft.DiaSymReader.Native (dotnet#21420)
  Resolving merge conflict
  Don't pick a project arbitrarily when navigating to symbols
@333fred
Fixed unintentional capitalization change.
21c2416
@333fred
Copy link
Member Author

333fred commented Aug 15, 2017

Ping @AlekseyTs @jaredpar @dotnet/roslyn-compiler for a couple of reviews please.

@333fred
Merge remote-tracking branch 'dotnet/features/ioperation' into remove…
1eb79ff 
…-literal-text

* dotnet/features/ioperation: (77 commits)
  Refactor variable names.
  Rename parameters.
  renamed methods.
  Rename method.
  Move integration test machines to 15.3 RTM (dotnet#21535)
  Update comment
  Fixed unintentional capitalization change, corrected grammar.
  Do a better job with leading trivia on types when moving types to a new file.
  Fixed unintentional capitalization change.
  Remove Release subdir from template output path (dotnet#21482)
  Fix bug in 'this' optimization for classes (dotnet#21510)
  Add test.
  Improve find-refs behavior when the user invokes it with a symbol selected.
  Fix regression in VB attribute classification.
  Fixed unintentional capitalization change.
  fix merge issues
  OSS sign debugger binaries. (dotnet#21468)
  More test fixes.
  Fixed misgenerated tests.
  Use default tuple fields in conversion since fields from inferred names are marked not usable in C#7
  ...
@333fred
Fix lock test.
bbf56e3
@333fred
Copy link
Member Author

333fred commented Aug 17, 2017

@dotnet-bot retest ubuntu_14_debug_prtest please

@333fred
Copy link
Member Author

333fred commented Aug 21, 2017

Ping for reviews.

AlekseyTs
AlekseyTs approved these changes Aug 21, 2017
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@333fred
Merge remote-tracking branch 'dotnet/features/ioperation' into remove…
d01fef0 
…-literal-text

* dotnet/features/ioperation:
  Fix no completion in partially written code before 'await'.
  parameter refness update in local functions/lambda should be caught a… (dotnet#21573)
  Don't offer to use pattern matching if a user defined operator was involved.
  Fix crash in VisualBasic.Binder.MemberLookup.AddLookupSymbolsInfoInTypeParameter when it is called with Cref TypeParameter. (dotnet#21586)
  Additional refactors.
  Refactor IConditionalChoiceExpression.
  use PerformIO utility
  put dispose under finally.
  Update to Microsoft.DiaSymReader.PortablePdb, Microsoft.DiaSymReader.Converter.Xml to 1.4.0-beta1-62016-01 (dotnet#21557)
  removed usage of ImmutableArray in json.net
  Update PULL_REQUEST_TEMPLATE.md
  Add test documentation to the PR template
  Remove LocalFunctionRewriting pass (dotnet#21408)
  PR feedbacks
  3 product changes and 1 test change
@333fred 333fred merged commit 0e44aae into dotnet:features/ioperation Aug 22, 2017
@333fred 333fred deleted the remove-literal-text branch August 22, 2017 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@mavasani mavasani mavasani approved these changes

@cston cston cston approved these changes

Assignees
No one assigned
Labels
Area-Analyzers Area-Compilers cla-already-signed Feature - IOperation IOperation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@333fred @cston @jaredpar @CyrusNajmabadi @AlekseyTs @mavasani @dnfclas