[PATCH] D115031: [AST] Print NTTP args as string-literals when possible

[Aaron Ballman via Phabricator via cfe-commits]

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