tahonermann requested changes to this revision. tahonermann added inline comments. This revision now requires changes to proceed.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:118-120 +def ext_concat_predef_ms : ExtWarn< + "concatenation of predefined identifier '%0' is a Microsoft extension">, + InGroup<MicrosoftConcatPredefined>; ---------------- I think it makes since to be more explicit that the concatenation is with regard to string literals. ================ 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__; +} ---------------- ================ Comment at: clang/include/clang/Parse/Parser.h:578-582 + bool isTokenConcatenable() const { + return isTokenStringLiteral() || + getLangOpts().MicrosoftExt && + tok::isMSPredefinedMacro(Tok.getKind()); + } ---------------- 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. ================ Comment at: clang/include/clang/Sema/Sema.h:5681 + std::vector<Token> ExpandUnexpandableMsMacros(ArrayRef<Token> Toks); ExprResult BuildPredefinedExpr(SourceLocation Loc, ---------------- ================ 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; + } ---------------- 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. ================ 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. ---------------- I think `__func__` is not considered a string literal in Microsoft mode, so I think this comment needs some adjusting. ================ 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"); ---------------- What is the reason for requiring non-concatenated predefined function local macros to be handled by `ActOnPredefinedExpr()`? ================ Comment at: clang/lib/Sema/Sema.cpp:1494 +Decl *Sema::getCurScopeDecl() { + if (const BlockScopeInfo *BSI = getCurBlock()) ---------------- `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). ================ 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. ---------------- ================ Comment at: clang/lib/Sema/SemaExpr.cpp:2001-2004 + OS << '"' + << Lexer::Stringify(PredefinedExpr::ComputeName( + getPredefinedExprKind(Tok.getKind()), currentDecl)) + << '"'; ---------------- 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? ================ Comment at: clang/lib/Sema/SemaExpr.cpp:3694 + Decl *currentDecl = getCurScopeDecl(); + if (isa<TranslationUnitDecl>(currentDecl)) { Diag(Loc, diag::ext_predef_outside_function); ---------------- This can just be a `nullptr` check with my suggested change to `getCurScopeDecl()`. ================ Comment at: clang/test/Sema/ms_predefined_expr.cpp:62 + static_assert(__FUNCTION__ ""_len == 14); // expected-warning{{concatenation of predefined identifier '__FUNCTION__' is a Microsoft extension}} } ---------------- If we don't already have such tests, please add tests that exercise these macros at global, namespace, and class scope. 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