tahonermann added inline comments.
================
Comment at: clang/lib/Lex/Lexer.cpp:3378-3379
- if (LooseMatch)
+ // If no diagnostic has been emitted yet we do not want to recover here
+ // to make sure this function will be called again and a diagnostic emitted
then.
+ if (LooseMatch && Diagnose)
----------------
cor3ntin wrote:
> tahonermann wrote:
> > I'm having a hard time understanding what this comment is trying to convey.
> > The comment response to Aaron was much more helpful to me. How about this
> > suggested edit?
> It's not that a diagnostic would be emitted twice, is that it would not be
> emitted at all.
> By recovering - ie returning a loose match - we prevent the function to be
> called again on the same buffer.
> So if during the first call Diagnose is false (because we were called from a
> tryXXXX function), then the compilation will continue assuming we were in
> fact able to parse an identifier and never informs us of an invalid name.
More evidence of the difficulty I'm having appreciating this comment :)
Does this sound right?
// Only map a loose match to a code point when in a diagnosing state.
// If a mapped code point were to be returned in a non-diagnosing state,
// token caching would prevent the opportunity to issue a diagnostic when
// the token is later used.
Tangent: It might be worth renaming one of `Res` and `Result`. I keep getting
confused due to the similar names. Perhaps rename `Result` to `Token` or
`ResultToken`?
================
Comment at: clang/lib/Lex/Lexer.cpp:3386
+
+ if (!Result || CurPtr - StartPtr == (ptrdiff_t)(Buffer.size() + 4))
StartPtr = CurPtr;
----------------
cor3ntin wrote:
> cor3ntin wrote:
> > tahonermann wrote:
> > > This is a bit of a tangent, but under what circumstances would `CurPtr -
> > > StartPtr` not be equal to `Buffer.size() + 4`? Actually, I'm not sure
> > > that +4 is correct. It looks like `StartPtr` is expected to point to `N`
> > > at the beginning of the function, so the initial `\` is not included in
> > > the range. If `N` isn't seen, the function returns early. Likewise, if
> > > either of the `{` or `}` delimiters is not found, the function returns
> > > early.
> > >
> > > I think the goal of this code is to skip the `getAndAdvanceChar()`
> > > machinery when the buffer is being claimed (no need to parse UCNs or
> > > trigraphs in the character name), but it looks like, particularly with
> > > this DR, that we might always be able to do that.
> > afaict, 4 is correct here because we are one past-the-end.
> > I do however agree that this whole pattern which is used a few times is
> > probably unnecessary, i do think it would be good to investigate. Not in
> > this patch though, imo
> I looked into it, I'm sure we could improve but not easily,
> `getAndAdvanceChar` does set some flags on the token in the presence of
> trigraphs/line splicing and we need those flags to be set, this is the reason
> we do need to call that method.
> It's not super efficient but it's such an edge case... I'd rather not touch
> that now
My concern is that, as is, the code always takes the `else` branch (except when
`Result` is non-null). Assuming a buffer containing "X", the pointers are
arranged as follows (where `$` is one past the end).
\ N { X } $
| | `- CurPtr
| `- Buffer
`- StartPtr
`CurPtr - StartPtr` is 4, but `Buffer.size() + 4` is 5 (`Buffer.size()` is 1 in
this case).
I think there might be an easy test to see if this is working as intended. If
it isn't, I would expect a diagnostic to be issued if trigraphs are enabled and
the buffer contains a trigraph sequence. Something like:
\N{LOTUS??>}
================
Comment at: clang/test/Preprocessor/ucn-pp-identifier.c:141
+// expected-warning@-2 {{incomplete delimited universal character name}}
#ifdef TRIGRAPHS
----------------
How about adding a test for an escaped new line? I think this is currently UB
according to the standard ([[ http://eel.is/c++draft/lex.phases#1.2 |
(lex.phases)p2 ]]), but I believe you are trying to change that with [[
https://wg21.link/p2621 | P2621 ]].
int \N{\
LATIN CAPITAL LETTER A WITH GRAVE};
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138861/new/
https://reviews.llvm.org/D138861
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
