RIscRIpt added inline comments.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1310-1311
+ case tok::kw_L__FUNCSIG__: // primary-expression: L__FUNCSIG__ [MS]
+ if (!getLangOpts().MicrosoftExt ||
+ !TokenIsLikeStringLiteral(NextToken(), getLangOpts())) {
+ Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
----------------
tahonermann wrote:
> `TokenIsLikeStringLiteral()` checks `MicrosoftExt`, so the check here is
> redundant. This is an example of why I would like to see the `MicrosoftExt`
> checking pushed down to `isFunctionLocalPredefinedMsMacro()`; that really
> seems where that checking belongs to me.
Let's consider this code:
```
if (!TokenIsLikeStringLiteral(NextToken(), getLangOpts())) {
Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
ConsumeToken();
break;
}
```
The condition can be expanded as follows: `!(isStringLiteral || (MsExt &&
NeededToken))`. Let's consider `MsExt` is `false`, then we have:
`!isStringLiteral`. So, if next token is StringLiteral we will try to
concatenate it even without MsExt, which is invalid. Thus this seemingly
redundant check is needed.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:3298-3299
+ assert(
+ (StringToks.size() != 1 ||
+ !tok::isFunctionLocalPredefinedMsMacro(StringToks[0].getKind())) &&
+ "single predefined identifiers shall be handled by ActOnPredefinedExpr");
----------------
tahonermann wrote:
> Is there a missing check for `MicrosoftExt` here? This is another example of
> why I would prefer to see those checks pushed down to
> `isFunctionLocalPredefinedMsMacro()`.
My intention for this `assert` is to ensure that we keep treating single
predefined identifiers as `PredefinedExpr` (when we don't need to concat them)
to preserve AST. I've adjusted the check to make it more clear.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:1300-1307
+ if (!getLangOpts().MicrosoftExt || !tok::isUnexpandableMsMacro(SavedKind)
||
+ !tok::isUnexpandableMsMacro(NextToken().getKind()) &&
+ !tok::isStringLiteral(NextToken().getKind())) {
+ Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
+ ConsumeToken();
+ break;
+ }
----------------
RIscRIpt wrote:
> tahonermann wrote:
> > Since the conditional code is only relevant for some of the tokens here, I
> > would prefer to see the other token kinds (`kw___func__`,
> > `kw___PRETTY_FUNCTION__`) handled separately to avoid the conditional
> > fallthrough. That means duplicating the calls to
> > `Actions.ActOnPredefinedExpr()` and `ConsumeToken()` between the blocks,
> > but that bothers me less than having to understand the complicated
> > condition.
> That's a valid point. And by handling two tokens separately allows
> simplification of the condition. Adjusted code.
Having `TokenIsLikeStringLiteral` I think we can return to single case. I
believe 3 values in condition is not difficult.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153914/new/
https://reviews.llvm.org/D153914
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
