Skip to content

Conversation

@AnthonyLatsis
Copy link
Collaborator

Follow-up to #39572

The range syntax is now ([+-]?N:)?N-([+-]?N:)?N where N is [0-9]+.

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

Thank you! Just a few comments about code styling. Implementation looks pretty good

Copy link
Member

Choose a reason for hiding this comment

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

This is a little bit confusing. I feel std::make_pair(LineColumnRange::NoValue, firstValue) is more clear.

Comment on lines 412 to 419
Copy link
Member

Choose a reason for hiding this comment

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

auto lineNo = DiagnosticLineNo;
switch (lineOffsetKind) {
case ::None
  lineNo = firstValue;
  break;
case ::Plus
  lineNo += firstValue;
  break;
case ::Minus
  lineNo -= firstValue;
  break;
}

WDYT?

Comment on lines 356 to 372
Copy link
Member

Choose a reason for hiding this comment

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

Clever, but

  const enum class LineOffsetKind : uint8_t {
    None,
    Plus,
    Minus
  };

  LineOffsetKind lineOffsetKind = LineOffsetKind::None;
  switch (Str.front()) {
  case '+':
    lineOffsetKind = LineOffsetKind::Plus;
    Str = Str.drop_front();
    break;
  case '-':
    lineOffsetKind = LineOffsetKind::Minus;
    Str = Str.drop_front();
    break;
  default:
    break;
  } 

feels easier to read.

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis Feb 18, 2022

Choose a reason for hiding this comment

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

I had the feeling this was gonna bug someone 😄

No problem, can I keep the lambda for constness though? When reading C++, I feel a ton of relief every time I see that something's not going to be mutated in the next 50+ lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, never mind; the difference looks negligible this time.

@rintaro
Copy link
Member

rintaro commented Feb 18, 2022

@swift-ci Please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@rintaro Appreciate the review!

@AnthonyLatsis AnthonyLatsis merged commit b296d41 into swiftlang:main Feb 18, 2022
@AnthonyLatsis AnthonyLatsis deleted the fixit-line-offsets branch February 18, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants