[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked 3 inline comments as done.
tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:445-446
+    const CharacterLiteral *E) {
+  if (Optional<PrimType> T = classify(E->getType()))
+    return this->emitConst(E, E->getValue());
+
----------------
tahonermann wrote:
> `T` isn't used and `classify()` already returns an `Optional<PrimType>`, so no conversion coercion appear to be intended here. I think this can be simplified to:
>   if (classify(E))
>     this->emitConst(E, E->getValue());
Good point. Since the value of the expression is always an `APInt`, might was well skip the `classify()` altogether. `emitConst()` does the classify anyway.


================
Comment at: clang/test/AST/Interp/arrays.cpp:143
+
+};
----------------
cor3ntin wrote:
> tahonermann wrote:
> > As others already noted, additional testing of multicharacter literals and UCNs (including named universal characters like `\N{LATIN_CAPITAL_LETTER_E}` would be beneficial. Some tests of character escapes like `\t` wouldn't hurt either.
> > 
> > Clang does not yet support use of `-fexec-charset` to set the literal encoding (execution character set) to anything other than UTF-8 though work on that has been done (see D93031). If such work was completed, it would be useful to run some tests against a non-UTF-8 encoding. Maybe next year.
> Yes, wide **multicharacter** literals, that's was important information missing, thanks for spotting that.
> 
> I have mixed feeling about adding tests for escape sequences.  Their replacement doesn't happen during constant evaluation.
> We shouldn't replicate the lexing tests here.
> 
> but we should compare string literal with byte values. Testing a string literal against another one doesn't ensure the code units are correct if both are equally miss evaluated.
> 
> Also we could add explicit tests for null termination here as they are added as part of evaluation in theory - but then again that's also something clang does earlier.
> 
> If we want we could consider enabling the byte code interpreter on the existing lexing test files, i actually think that's the better way to deal with the escape sequences tests.
I changed the first test that inspects all characters of a string to comparing with integers instead. Do you have a suggestion for what lexing tests to enable the constant interpreter in?


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

https://reviews.llvm.org/D135366