[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D124556#3482224 , @ken-matsui 
wrote:

> Thank you!

Thanks in turn.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG52ce95a1a554: [NFC] Prevent shadowing a variable declared in 
`if` (authored by ken-matsui, committed by hubert.reinterpretcast).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

Files:
  clang/lib/Basic/Diagnostic.cpp


Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -983,13 +983,13 @@
   if (const char *S = tok::getPunctuatorSpelling(Kind))
 // Quoted token spelling for punctuators.
 Out << '\'' << S << '\'';
-  else if (const char *S = tok::getKeywordSpelling(Kind))
+  else if ((S = tok::getKeywordSpelling(Kind)))
 // Unquoted token spelling for keywords.
 Out << S;
-  else if (const char *S = getTokenDescForDiagnostic(Kind))
+  else if ((S = getTokenDescForDiagnostic(Kind)))
 // Unquoted translatable token name.
 Out << S;
-  else if (const char *S = tok::getTokenName(Kind))
+  else if ((S = tok::getTokenName(Kind)))
 // Debug name, shouldn't appear in user-facing diagnostics.
 Out << '<' << S << '>';
   else


Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -983,13 +983,13 @@
   if (const char *S = tok::getPunctuatorSpelling(Kind))
 // Quoted token spelling for punctuators.
 Out << '\'' << S << '\'';
-  else if (const char *S = tok::getKeywordSpelling(Kind))
+  else if ((S = tok::getKeywordSpelling(Kind)))
 // Unquoted token spelling for keywords.
 Out << S;
-  else if (const char *S = getTokenDescForDiagnostic(Kind))
+  else if ((S = getTokenDescForDiagnostic(Kind)))
 // Unquoted translatable token name.
 Out << S;
-  else if (const char *S = tok::getTokenName(Kind))
+  else if ((S = tok::getTokenName(Kind)))
 // Debug name, shouldn't appear in user-facing diagnostics.
 Out << '<' << S << '>';
   else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@hubert.reinterpretcast

I see. Thank you for the detailed information!

Could you please commit this patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D124556#3480228 , @ken-matsui 
wrote:

> @hubert.reinterpretcast
>
> Sorry, I'm a newbie here, but is there anything I should do after getting 
> approved?

I'm not sure if the instructions are a bit out-of-date: 
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

If you intend to contribute going forward, the idea would be to get a few 
patches approved and committed on your behalf and then to request commit 
access. Until you have commit access, you will need to ask people on the review 
to help you commit.

I can commit this patch for you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@hubert.reinterpretcast

Sorry, I'm a newbie here, but is there anything I should do after getting 
approved?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

Thank you for your review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.
This revision is now accepted and ready to land.

LGTM; thanks for the patch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@hubert.reinterpretcast,

Thank you for your suggestion! I’ve updated the summary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D124556#3479833 , @ken-matsui 
wrote:

> @hubert.reinterpretcast,
>
> Sorry to have missed providing a summary.

You can still provide one by using the "Edit Revision" link.

I suggest something like:
Prevents confusion over which `S` is referenced in the final `else` branch if 
such a use is added.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@hubert.reinterpretcast,

Sorry to have missed providing a summary.

In most cases, shadowing a declaration of a local variable should be avoided to 
prevent making others confused and mistakes because we need to figure out the 
boundaries of the declaration. In this case, the variable declared by `const 
char *S` in `if` can be accessed by every branch. That means that declarations 
in `else if` had shadowed the declaration in `if`, so I changed them to use the 
same declaration used in `if`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124556: [NFC] Prevent shadowing a variable declared in `if`

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

@ken-matsui, can you provide some rationale for the change (got compiler 
warning/error)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124556/new/

https://reviews.llvm.org/D124556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits