[PATCH] D129951: adds `__disable_adl` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: erichkeane, clang-language-wg.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4132
+def DisableADL : InheritableAttr {
+  let Spellings = [Keyword<"__disable_adl">];
+  let Subjects = SubjectList<[Function]>;
----------------
cjdb wrote:
> rsmith wrote:
> > Has this syntax been discussed already? If not, why did you choose keyword syntax instead of a normal attribute?
> Aaron and I discussed this offline. I'd originally intended to do `[[clang::disable_adl]]`, but Aaron pointed out that this would be problematic for all involved if someone uses a library in GCC, since the feature is implicitly turned off. If we use this syntax, then it forces libraries to acknowledge (and possibly document) that ADL is restricted on Clang, but potentially not GCC.
> 
> I will add a note to the documentation that I'm promising.
Yup! The basic thinking I had is: if this attribute is ignored by an implementation, it's very possible that the user's code may change behavior -- if they're unlucky, it's a silent behavior change. However, if it's a keyword, then there's a syntax error and no chance for a silent behavior change.

If you think we need a more broad discussion of the syntax, that'd be fine by me -- it's possible there's a policy question hiding in here about when we want keywords and when we are fine with regular attribute syntax (type attributes are another source of these kinds of questions).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6851
+def DisableADLDocs : Documentation {
+  let Content = [{Please don't LGTM without this being fully documented.}];
+}
----------------
rsmith wrote:
> OK!
Btw, as a reviewer: thank you for this. I appreciate clear markings of "we're not done yet!" in the source.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:542
 If a statement is marked ``nomerge`` and contains call expressions, those call
-expressions inside the statement will not be merged during optimization. This 
+expressions inside the statement will not be merged during optimization. This
 attribute can be used to prevent the optimizer from obscuring the source
----------------
cjdb wrote:
> shafik wrote:
> > A lot of these look like unrelated changes.
> I tend to not argue with autoformatters.
We still ask that you don't commit unrelated changes (this was stripping trailing whitespace, which is often an editor setting) -- it makes git blame harder than it needs to be. Feel free to commit the whitespace fixes before/after the patch if you'd like, though!


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5756
+  if (FunctionDecl *F = D->getAsFunction();
+      F->isOverloadedOperator() || F->isCXXClassMember()) {
+    S.Diag(AL.getLoc(), diag::err_disable_adl_no_operators)
----------------
cjdb wrote:
> rsmith wrote:
> > Maybe also global `operator new` / `operator delete`?
> How does ADL even work in with the global namespace? Should it be invalid to apply here?
Should this be `isCXXInstanceMember()` instead? Or do we intend to disallow this on static member functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129951