[PATCH] D133853: [AST] Add msvc-specific C++11 attributes

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D133853#3795579 <https://reviews.llvm.org/D133853#3795579>, @aaron.ballman wrote:

> In D133853#3793284 <https://reviews.llvm.org/D133853#3793284>, @RIscRIpt wrote:
>
>> In D133853#3792518 <https://reviews.llvm.org/D133853#3792518>, @aaron.ballman wrote:
>>
>>> I'm wondering what the goal is for these changes. ... Are you intending to add semantics for these attributes in follow-up patches?
>>
>> To be honest, I wasn't planning to do any of follow-up patches. I made a patch for internal usage at my job, and decided to submit it upstream.
>> The main reason I (we) need this patch is that we need to be able to parse MSVC-specific code (in the current case - detect `constexpr` functions). Since Visual Studio 17.3 (MSVC 14.33.31629), Microsoft's STL library added `[[msvc::constexpr]]` attribute, which is not documented yet, but makes a function to act like a `constexpr` function: see this godbolt sample <https://godbolt.org/z/76fYq145d> (i.e. forbids non-constexpr statements inside).
>>
>> To make the patch complete, I decided to browse previous Microsoft's STL versions and see which vendor specific (`msvc::`) attributes they added previously; in this patch I added all attributes I was able to find.
>>
>>> We don't typically add attributes to Clang that don't have any effect unless there's a very compelling reason to do so.
>>
>> Theoretically, I could re-submit (or adjust this) patch, which would add support for `[[msvc::constexpr]]` attribute with semantic meaning of `constexpr` for functions (for code parsed with `-fms-extensions` flag). Regarding other attributes - unfortunately they are either poorly documented, or not documented at all, so I can drop commits for these attributes.
>>
>> *Edit:* Please, let me know how we can proceed - either I abandon the patch; or add support only for `[[msvc::constexpr]]`, or any other way of proceeding further?
>
> Thanks for the information! Roping in @erichkeane as attribute code owner to make sure he's on board with the idea, but my suggestion is to only support `[[msvc::constexpr]]` with the semantic meaning of `constexpr`. It's a good question as to whether we want to support that only when passing `-fms-extensions` or not (it seems like a generally useful attribute); we don't do the same for GNU attributes, but maybe we don't want to follow that pattern? This will be the first attribute we add with the `msvc` vendor namespace.
>
> If you find there's a good reason to upstream the other ones, we can certainly consider it. FWIW, one Microsoft-specific attribute I know people have been asking about is `[[msvc::no_unique_address]]`. See https://github.com/llvm/llvm-project/issues/49358 for more details. Not suggesting you're on the hook for that or anything, just pointing it out in case you wanted to work on that one at some point.

I agree with Aaron here, there isn't much value in the "do nothing" attributes, I believe the 'unknown attribute' warning for them is superior to an ignored attribute.

A functional `msvc::constexpr` would be acceptable, however, I'm having a difficult time figuring out the purpose of it, it seems like it is just a 'constepr keyword' replacement?



================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:296
+  // Validation is in Sema::ActOnAttributedStmt().
+  return ::new (S.Context) MSConstexprAttr(S.Context, A);
+}
----------------
Typically we try to do the ::Create function that is generated for attributes, rather than placement new.  I realize we are consistently inconsistent...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133853