tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.
================
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__;
+}
----------------
RIscRIpt wrote:
> tahonermann wrote:
> >
> Thanks, I like the name. But shouldn't we reflect that we are referring to
> only Microsoft (unexpandable) macros? How about
> `isFunctionLocalPredefinedMsMacro`?
I don't think the Microsoft association is meaningful. Sure, some of the
predefined macros are only treated differently when compiling in Microsoft
mode, but some of those macros aren't Microsoft specific. Also, there might be
macros provided by other implementations that we'll want to emulate some day.
================
Comment at: clang/include/clang/Sema/Sema.h:5681
+ std::vector<Token> ExpandUnexpandableMsMacros(ArrayRef<Token> Toks);
ExprResult BuildPredefinedExpr(SourceLocation Loc,
----------------
RIscRIpt wrote:
> tahonermann wrote:
> >
> Renamed to `ExpandFunctionLocalPredefinedMsMacros`. If you think my addition
> of `Ms` is redundant, let me know.
I don't think the `Ms` is redundant, but I do think it is unnecessary and
slightly confusing (`__FUNCTION__`) is not a Microsoft macro.
================
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);
----------------
`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.
================
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");
----------------
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()`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1979-1980
+ Decl *currentDecl = getCurLocalScopeDecl();
+ if (!currentDecl)
+ currentDecl = Context.getTranslationUnitDecl();
+ std::vector<Token> ExpandedToks;
----------------
This could use a comment since it isn't obvious why the translation unit
declaration is used when not in a local scope declaration of some kind.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:2001-2004
+ OS << '"'
+ << Lexer::Stringify(PredefinedExpr::ComputeName(
+ getPredefinedExprKind(Tok.getKind()), currentDecl))
+ << '"';
----------------
RIscRIpt wrote:
> 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`.
Ah, ok. And I see it even differentiates what name is produced for
`__PRETTY_FUNCTION__`. That leaves me wondering what the right behavior should
be at class and namespace scope, but maybe I'm better off not asking questions
I don't want to know the answer to.
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
