[PATCH] D115715: [clang-tidy] Fix llvm-header-guard for Windows paths containing drive letter (e.g. C:).

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

In D115715#3191742 <https://reviews.llvm.org/D115715#3191742>, @sberg wrote:

>> So the check, for a file called e.g. "C:\test\test.h" would suggest the guard C:_TEST_TEST_H being an invalid name due to the presence of the colon.
>
> ...though the new suggestion `C__TEST_TEST_H` isn't ideal either, in that it uses an identifier that is reserved for the implementation (due to the double underscore)

Indeed, I hadn't thought about it. We *might* change all `__+` into a single `_`, but that seems an overkill to me (or at least something for another patch).
Also, the problem is not limited to `:`. A file could be named `a--b.h` and it would get a guard `A__B_H` with a double underscore.

In D115715#3191767 <https://reviews.llvm.org/D115715#3191767>, @salman-javed-nz wrote:

> The problem at the heart of all this is that llvm-header-guard isn't written flexible enough to support non-LLVM project structures.

I know, but this check is still useful outside of LLVM.
>From my point of view, there's not much missing flexibility to use it outside LLVM (for basic stuff), only such small edge cases like this one.
Of course, one would wish to be able to set "root" names other than "llvm-project" and so on, but that's out of scope here.

> Edit: I'm assume you're seeing this problem outside of the LLVM project?

Yes, in the bug report, it is used outside of LLVM.



================
Comment at: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp:57
   std::replace(Guard.begin(), Guard.end(), '-', '_');
+  std::replace(Guard.begin(), Guard.end(), ':', '_');
 
----------------
salman-javed-nz wrote:
> salman-javed-nz wrote:
> > Are there other characters we should be sanitising here?
> > (Lest keep revisiting this code to add more characters to the list)
> Typo:
> *Lest **we** keep revisiting
Well, difficult to say. On Windows, there are pretty many allowed characters in filenames, but I don't think we should care about it.
I think we should just care about what is sometimes called "POSIX Fully portable filenames" (which contains: A-Z a-z 0-9 . _ -).
Colon `:` is special (similarly to slashes `/` and `\`) as it may appear in the path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115715