aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
Aside from a testing nit, this LGTM! ================ Comment at: clang/include/clang/Basic/CharInfo.h:199-200 + return "\\t"; + case '\v': + return "\\v"; + } ---------------- lichray wrote: > aaron.ballman wrote: > > lichray wrote: > > > aaron.ballman wrote: > > > > We're also missing `\?` right? > > > `?` does not seem to need `"escaping?"` > > It's the only simple escape sequence we're not handling here: > > http://eel.is/c++draft/lex.literal#nt:simple-escape-sequence-char > > > > (You need to escape `?` thanks to trigraphs. Consider a string literal like > > `"This does what now??!"`.) > > It's the only simple escape sequence we're not handling here: > > http://eel.is/c++draft/lex.literal#nt:simple-escape-sequence-char > > > > (You need to escape `?` thanks to trigraphs. Consider a string literal like > > `"This does what now??!"`.) > > Hmm, I think we're safe here > ``` > ~/devel/llvm-project> ./build/bin/clang++ -fsyntax-only -std=c++14 c.cc > c.cc:1:50: warning: trigraph converted to '|' character [-Wtrigraphs] > void foo() { char const s[] = "This does what now??!"; } > ^ > 1 warning generated. > ~/devel/llvm-project> ./build/bin/clang++ -fsyntax-only -std=c++20 c.cc > c.cc:1:50: warning: trigraph ignored [-Wtrigraphs] > void foo() { char const s[] = "This does what now??!"; } > ^ > 1 warning generated. > ``` > For these reasons, > 1. I don't want users to copy diagnosis messages, paste them into a program, > and get something different. This has been prevented by the warnings. > 2. We support trigraph up to C++14, way behind the version (C++20) we can use > string literals in NTTP. So it should be mostly fine. > 3. In C++14 we can use the characters in NTTP, but that one has a different > diagnosis: > > ``` > template <char C> class ASCII {}; > > void Foo(ASCII<'?'>); > void Foo(ASCII<'??!'>); > void test_pascal() { > ASCII<'!'> a; > Foo(a); > } > ``` > > ``` > c.cc:4:17: warning: trigraph converted to '|' character [-Wtrigraphs] > void Foo(ASCII<'??!'>); > ^ > c.cc:7:3: error: no matching function for call to 'Foo' > Foo(a); > ^~~ > c.cc:3:6: note: candidate function not viable: no known conversion from > 'ASCII<'!' aka 33>' to 'ASCII<'?' aka 63>' for 1st argument > void Foo(ASCII<'?'>); > ^ > c.cc:4:6: note: candidate function not viable: no known conversion from > 'ASCII<'!' aka 33>' to 'ASCII<'|' aka 124>' for 1st argument > void Foo(ASCII<'??!'>); > ^ > 1 warning and 1 error generated. > ``` > Looks nice to me even if we don't escape. Okay, that sounds reasonable to me, thank you for checking! Can you add a trigraph test case so that it's clear we purposefully don't escape the `\?` (with some comment to that effect)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115031/new/ https://reviews.llvm.org/D115031 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
