[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28
+/// Replace invalid characters in the identifier with '_'.
+static std::string replaceInvalidChars(StringRef Identifier) {
+  std::string Fixed{Identifier};
----------------
salman-javed-nz wrote:
> aaron.ballman wrote:
> > This fixes some issues but I think it introduces new issues too. The old code would accept Unicode characters and pass them along as (valid) identifier characters. The new code will replace all the Unicode characters with `_` which means some people's header guards may go from useful to `_______________`. We should definitely add test cases for Unicode file names.
> > 
> > Ultimately, I think this functionality will want to use `UnicodeCharSets.h` to implement something along the lines of what's done in `isAllowedInitiallyIDChar()` and `isAllowedIDChar()` from Lexer.cpp.
> `isAllowed*IDChar()`does exactly what I want, but is a static function in Lexer.cpp.
> Should I be changing its declaration so that it's exposed via Lexer.h?
> I don't know what the bar is to warrant changing core public Clang API. 
> 
> If I can just call the function directly from llvm-header-guard, I can avoid code duplication.
> 
> 
We could expose the interface from `Lexer` if we feel that's the right approach. I was going back and forth on whether it was the right approach and eventually thought that we don't want to expose it directly because I don't think we want clang-tidy to care about things like `LangOpts.AsmPreprocessor` or even `LangOpts.DollarIdents`.

So my recommendation is to put this into LexerUtils.h/.cpp in clang-tidy rather than changing the Clang Lexer interface. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149