Removal of Unnecessary Overloads + Extension method tweaks.

[AdamSpeight2008]

No description provided.

[mikedn]

I don't know if matters here but removing the non-params overloads leads to an unnecessary empty array allocation if no additional arguments are provided. The DiagnosticInfo class that ends up storing the args is prepared to deal with null arrays and that may be a sign that the overloads were intentional.

As for IsBetween, well, that sounds like a reasonable attempt at killing the lexer performance 😄

[AdamSpeight2008]

@mikedn Why would it create an array? If no parameters are passed should the value of the params be null since an array is class type.

Of all the cases (VS can find) it is passed only one argument a string, an array see like to much.

[mikedn]

It creates an array of 0 length because that's what the C# specification requires.

I searched the code a bit and there are a couple of uses of the non-params overloads. Even if there are no concerns about allocation I'd probably keep those overloads because they avoid bloating the call sites with useless code for the array allocation.

[AdamSpeight2008]

Why not optional argument?

 protected void AddError(int position, int width, ErrorCode code, object[] args = null)

Plus an overload for the case of string

 protected void AddError(int position, int width, ErrorCode code, string arg)

See commit 65d3d0a

[mikedn]

Seems like too much trouble. Avoiding allocating an empty array at the expense of a couple of null checks and trivial overloads is one thing. Avoiding allocating an array with a single element at the expense of additional overloads and more code to deal with this special case is another thing and should only be done if there's evidence that it improves performance.

Also, your PR contains a bunch of unrelated and unnecessary formatting changes. In particular, you seem to indent the code with 2 spaces while this project is using 4.

[AdamSpeight2008]

Then if that's the case why the class OneOrMany?

[mikedn]

Presumably because someone has done the necessary analysis and decided that it's worth the trouble. I've never said that one should never do such an optimization, I said that such optimizations should only be done when they're really needed.

You started your PR with the goal to cleanup some code and when I pointed out that a particular change might be a de-optimization you changed the goal from cleanup to performance improvement, even if there's no evidence that the improvement is necessary.

[paulomorgado]

Changing:

       sb.AppendFormat("Summary: {0}\n", Kind);

into:

        sb.AppendLine($"Summary: {Kind}");

might seem pretty and modern but, in reality, it's changing into:

        sb.AppendLine(string.Formt("Summary: {0}", Kind));

Behind the curtain, string.Format uses a StriungBuilder. That would mean an allocation of a new StringBuilder when one is already being used.

The same goes to almost all, if not all, uses of string interpolation.

[paulomorgado]

Is there anywhere else in the Roslyn code where optional arguments are being used on public or protected methods?

[thorgeirk11]

The previous error said it was a binary operator while this error states that it is a unary operator.

[AdamSpeight2008]

I'm still trying to get used to Git. I thought that a pull request would only pull up to a specific point of time, not subsequent commits. It was only for the overloads and extensions.

[jaredpar]

Nit: blank line separator

[jaredpar]

Nit: double blank line

[jaredpar]

Thank you for the contribution!

A significant portion of this change is simply altering the style of the existing code. In general we are going to be reluctant to take such changes. They generally represent churn in the code base that don't provide concrete value.

Overall our code base now follows the .NET Foundation contributing guidelines including their coding style requirements. Much of the code didn't follow this when originally posted but over the last few weeks it has all been migrated to meet those guidelines. The code will continue to follow those guidelines going forward.

Style changes will generally be taken only when it:

• Significantly improves the readability of the code in question.
• Changes code to conform to the stated guidelines which previously did not.

[AdamSpeight2008]

@jaredpar The single line if's do improve readability.
Maybe allow them if it is small ( eg with the total line width of 80 chars?)
Eg Guards

if( arg0 == null ) throw new NullArgumentException( nameof(arg0));
if( arg1 == null ) throw new NullArgumentException( nameof(arg1));

More of guards and you are talk in the region of half the screen height, before you even get to the "good stuff" of the method. not all of have huge monitors, some laptops has more horizontal space than vertical

Simple returns

if( x == 0 ) return "Zero";

SImple Cases

select ( value )
{
  case 0:  return "Zero";
  case 1:  return "One";
  case 2:  return "Two";
  case 3:  return "Three";
  case 4:  return "Four";
  case 5:  return "Five";
  case 6:  return "Six";
  case 7:  return "Seven";
  case 8:  return "Eight";
  case 9:  return "Nine";
  default:
    return "shrug";
}

15 LoC vs 25 (separate lines ) or 35 if in require the breaks.
The 4 space indentation is too wide, it should be 2. Considering the each block / scope get indented.

Now consider the case where pattern-matching is included, the code will get both wider and longer (in terms of structural layout).

[jaredpar]

@AdamSpeight2008 whether or not single line ifs, or most other style decisions, improve readability is subjective. What is preferable to one developer may not be preferable to the other. The important aspect of style is to keep it consistent across the code base.

In this specific case though single line ifs are not permitted by the coding guidelines.

[sharwell]

@AdamSpeight2008 You submitted a pull request from your master branch. This means any subsequent commits you make to the master branch in your fork while this pull request is open will automatically appear here. To make things much easier, you should create a new branch in your fork and use it to perform one specific change (e.g. changing instances of AppendFormat to use string interpolation). You can then send a pull request to merge that branch and it can be evaluated independently of any other type of changes you want to propose.

To start working on a different proposed change, such as creating new constants for string literals that appear in code, go back to the latest master commit and create another new branch for that change.

Otherwise, when you send a bunch of different types of changes in a single pull request, it has a high likelihood of getting rejected if people disagree with one of the changes, even if they agree with the majority.

[AdamSpeight2008]

Note: The original repo for this PR as been removed.