aaron.ballman added a comment. In D115715#3191842 <https://reviews.llvm.org/D115715#3191842>, @curdeius wrote:
> 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). Fixit suggestions should not produce incorrect code and double underscores will introduce undefined behavior in C++ code. > 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. It could be made to be useful outside of LLVM, but as it stands today, the check is only intended to be useful for LLVM. If we want to make it useful outside of LLVM, that's fine, but there's a bit more to do (for example, the check should be exposed outside of the `llvm` module) and that includes generalizing the check. ================ Comment at: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp:57 std::replace(Guard.begin(), Guard.end(), '-', '_'); + std::replace(Guard.begin(), Guard.end(), ':', '_'); ---------------- curdeius wrote: > 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. Huh, I thought we had covered colons as part of the previous review -- sorry for missing that! FWIW, there are other characters that may appear as part of a path. `/` and `\` are both path separators, but `?` can appear in file namespace paths (which means `.` can then appear as part of the file name rather than a separator). > I think we should just care about what is sometimes called "POSIX Fully > portable filenames" (which contains: A-Z a-z 0-9 . _ -) I'd be pretty opposed to anything that takes non-ASCII characters and converts them to `_` as that will wind up with header guards like `________` for some users. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115715/new/ https://reviews.llvm.org/D115715 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
