RIscRIpt added inline comments.
================ Comment at: clang/include/clang/Basic/TokenKinds.h:87-93 +/// Return true if this token is a predefined macro +/// unexpandable by MSVC preprocessor. +inline bool isUnexpandableMsMacro(TokenKind K) { + return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ || + K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ || + K == tok::kw___FUNCDNAME__; +} ---------------- tahonermann wrote: > Thanks, I like the name. But shouldn't we reflect that we are referring to only Microsoft (unexpandable) macros? How about `isFunctionLocalPredefinedMsMacro`? ================ Comment at: clang/include/clang/Parse/Parser.h:578-582 + bool isTokenConcatenable() const { + return isTokenStringLiteral() || + getLangOpts().MicrosoftExt && + tok::isMSPredefinedMacro(Tok.getKind()); + } ---------------- tahonermann wrote: > RIscRIpt wrote: > > cor3ntin wrote: > > > Unless you find a better name, I think it's preferable to keep > > > `isTokenConcatenable` and `isTokenPredefinedMsMacro` as separate > > > functions. > > > Also, this seems like a weird place to check for > > > `getLangOpts().MicrosoftExt` > > Regarding `getLangOpts().MicrosoftExt`. If you are talking about it's > > presence in a function which name is meant to be used as a predicate, I > > agree. If you are talking about `class Parser`, then there're other places > > with references to `getLangOpts()`. > > > > Without such function `ParseStringLiteralExpression` implementation would > > be too verbose. > > Let's decide what we can do after I address other comments. > > > > Meanwhile, I renamed it to `isTokenLikeStringLiteral()`. > I suggest passing `getLangOpts()` to `isFunctionLocalPredefinedMacro` > (`isUnexpandableMsMacro`) and letting it determine whether the token is or is > not one of these special not-really-a-string-literal macro things. This will > facilitate additional such macros controlled by different options while also > colocating the option check with the related tokens. @tahonermann, in theory it sounds good, but if I understood you correctly, the implementation seem weird to me: ``` inline bool isFunctionLocalPredefinedMsMacro(TokenKind K, const LangOptions& langOpts) { if (!langOpts.MicrosoftExt) return false; return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ || K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ || K == tok::kw___FUNCDNAME__; } ``` And I would have to add `#include "LangOptions.h"` in `TokenKinds.h`. ================ Comment at: clang/include/clang/Parse/Parser.h:578-582 + bool isTokenConcatenable() const { + return isTokenStringLiteral() || + getLangOpts().MicrosoftExt && + tok::isMSPredefinedMacro(Tok.getKind()); + } ---------------- RIscRIpt wrote: > tahonermann wrote: > > RIscRIpt wrote: > > > cor3ntin wrote: > > > > Unless you find a better name, I think it's preferable to keep > > > > `isTokenConcatenable` and `isTokenPredefinedMsMacro` as separate > > > > functions. > > > > Also, this seems like a weird place to check for > > > > `getLangOpts().MicrosoftExt` > > > Regarding `getLangOpts().MicrosoftExt`. If you are talking about it's > > > presence in a function which name is meant to be used as a predicate, I > > > agree. If you are talking about `class Parser`, then there're other > > > places with references to `getLangOpts()`. > > > > > > Without such function `ParseStringLiteralExpression` implementation would > > > be too verbose. > > > Let's decide what we can do after I address other comments. > > > > > > Meanwhile, I renamed it to `isTokenLikeStringLiteral()`. > > I suggest passing `getLangOpts()` to `isFunctionLocalPredefinedMacro` > > (`isUnexpandableMsMacro`) and letting it determine whether the token is or > > is not one of these special not-really-a-string-literal macro things. This > > will facilitate additional such macros controlled by different options > > while also colocating the option check with the related tokens. > @tahonermann, in theory it sounds good, but if I understood you correctly, > the implementation seem weird to me: > ``` > inline bool isFunctionLocalPredefinedMsMacro(TokenKind K, const LangOptions& > langOpts) { > if (!langOpts.MicrosoftExt) > return false; > return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ || > K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ || > K == tok::kw___FUNCDNAME__; > } > ``` > And I would have to add `#include "LangOptions.h"` in `TokenKinds.h`. If you are still concerned about usage of LangOptions in this context, and at the same time you are not against of having such function, I can offer the following alternatives: 1. Make it a local lambda function in `ParseStringLiteralExpression` 2. Make it a TU local function in `ParseExpr.cpp` I went with (2) to do simplification of condition in switch statement below. Marking thread resolved, as it's no longer relevant. ================ Comment at: clang/include/clang/Sema/Sema.h:5681 + std::vector<Token> ExpandUnexpandableMsMacros(ArrayRef<Token> Toks); ExprResult BuildPredefinedExpr(SourceLocation Loc, ---------------- tahonermann wrote: > Renamed to `ExpandFunctionLocalPredefinedMsMacros`. If you think my addition of `Ms` is redundant, let me know. ================ 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; + } ---------------- 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. ================ Comment at: clang/lib/Parse/ParseExpr.cpp:3277-3280 + // String concatenation. + // Note that keywords like __func__ and __FUNCTION__ + // are not considered to be strings for concatenation purposes, + // unless Microsoft extensions are enabled. ---------------- tahonermann wrote: > I think `__func__` is not considered a string literal in Microsoft mode, so I > think this comment needs some adjusting. Right, removed mention of `__func__`. ================ Comment at: clang/lib/Parse/ParseExpr.cpp:3288-3291 + assert( + (StringToks.size() != 1 || + !tok::isUnexpandableMsMacro(StringToks[0].getKind())) && + "single predefined identifiers shall be handled by ActOnPredefinedExpr"); ---------------- tahonermann wrote: > What is the reason for requiring non-concatenated predefined function local > macros to be handled by `ActOnPredefinedExpr()`? To ensure generation of the same AST as before my changes (with `PredefinedExpr`): ``` | | `-VarDecl <col:2, col:19> col:13 a 'const char[2]' cinit | | `-PredefinedExpr <col:19> 'const char[2]' __FUNCTION__ | | `-StringLiteral <col:19> 'const char[2]' "f" ``` This actually brings a question: do we want to wrap StringLiterals (into PredefinedExpr) which were formed from concatenation of string-literals and some predefined identifier(s)? But it does not really make any sense: concatenated string does not represent any of PredefinedExpr. ================ Comment at: clang/lib/Sema/Sema.cpp:1494 +Decl *Sema::getCurScopeDecl() { + if (const BlockScopeInfo *BSI = getCurBlock()) ---------------- tahonermann wrote: > `getCurScopeDecl` isn't a great name since this doesn't handle class or > namespace scopes. How about `getCurLocalScopeDecl`? That name isn't quite > right either since this can return a `TranslationUnitDecl`, but I think it > matches the intent fairly well. > > None of the callers need the actual `TranslationUnitDecl` that is returned. I > think this function can return `nullptr` is the current scope isn't function > local and callers can issue the relevant diagnostic in that case (thus > avoiding the need to check that a translation unit decl was returned). Sounds good to me. The only alternative I can come up with is `getCurFunctionScopeDecl`, but is it any better? Adjusted code as per your suggestions. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1971-1974 + // MSVC treats some of predefined identifiers (e.g. __FUNCTION__) as + // expandable macros defined as string literals, which may be concatenated. + // Expand them here (in Sema), because StringLiteralParser (in Lex) + // doesn't have access to AST. ---------------- tahonermann wrote: > Changed, thank you! ================ Comment at: clang/lib/Sema/SemaExpr.cpp:2001-2004 + OS << '"' + << Lexer::Stringify(PredefinedExpr::ComputeName( + getPredefinedExprKind(Tok.getKind()), currentDecl)) + << '"'; ---------------- tahonermann wrote: > A diagnostic is issued if the call to `getCurScopeDecl()` returned a > translation unit declaration (at least in Microsoft mode). Does it make sense > to pass a pointer to a `TranslationUnitDecl` here? Shortly, yes, kind of. `ComputeName` can handle `TranslationUnitDecl` in which case it returns an empty string. This way we implicitly (without additional code) create empty string-literal Tokens which can be handled in `StringLiteralParser`. And we cannot pass non-string-literal tokens into `StringLiteralParser`. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3694 + Decl *currentDecl = getCurScopeDecl(); + if (isa<TranslationUnitDecl>(currentDecl)) { Diag(Loc, diag::ext_predef_outside_function); ---------------- tahonermann wrote: > This can just be a `nullptr` check with my suggested change to > `getCurScopeDecl()`. Done, but I had to undo removal of ``` currentDecl = Context.getTranslationUnitDecl(); ``` but it is not a big deal. 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