[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin 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__;
+}
----------------
RIscRIpt wrote:
> tahonermann wrote:
> > cor3ntin wrote:
> > > tahonermann wrote:
> > > > 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.
> > > I think it is, there is currently no way to link `isFunctionLocalPredefinedMacro` to the MS feature. "MSPredefinedMacro" is pretty self explanatory, same reason we most often - but not always - use GNU in the name of function related to GNU extensions.
> > > There are enough similar-sounding features and extensions that we should try to make it clear which feature we are talking about.
> > Perhaps we still haven't found the right name then. With the name that I've been advocating, this function should also return true for `tok::kw___PRETTY_FUNCTION__` even though that macro should not expand to something that can be concatenated with other string literals (whether compiling in Microsoft mode or not).
> > 
> > The Microsoft extension is the localized expansion to a string literal that can be concatenated with other string literals. We probably should isolate the conditions for that behavior to one place and if we do that, then I guess naming this function as specific to Microsoft mode is ok; we can always rename it later if it gets used for non-Microsoft extensions.
> > 
> > Here is my suggestion then:
> >   // Return true if the token corresponds to a function local predefined
> >   // macro that, in Microsoft modes, expands to a string literal (that can
> >   // then be concatenated with other string literals).
> >   inline bool isMsFunctionLocalStringLiteralMacro(TokenKind K, const LangOptions &langOpts) {
> >     return langOpts.MicrosoftExt && (
> >         K == tok::kw___FUNCTION__ || K == tok::kw_L__FUNCTION__ ||
> >         K == tok::kw___FUNCSIG__ || K == tok::kw_L__FUNCSIG__ ||
> >         K == tok::kw___FUNCDNAME__);
> >   }  
> > 
> > This will avoid the need for callers to check for `MicrosoftExt`; that is what I really want to avoid since it is easy to forget to do that check.
> 1. By requiring user to pass `LangOptions` I think we can remove MS association (implying that there could be other non-msft cases in the future)
> 2. We would have to include a `LangOptions.h` in `TokenKinds.h`, are we ok with this? Alternatively while this function is for msft cases only, we could pass `bool MicrosoftExt`
> 
> Personally, I like version with `LangOptions` and removal of `MS`.
> By requiring user to pass LangOptions I think we can remove MS association

I don't think there is any motivation to future proof against hypotheticals, we can always refactor later 

> We would have to include a LangOptions.h in TokenKinds.h

This makes me uneasy, but i think we can move the function to `LiteralSupport.h` and include that in `ParseExpr.cpp`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153914/new/

https://reviews.llvm.org/D153914