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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
