aaron.ballman marked an inline comment as done. aaron.ballman added a comment.
In D143670#4116858 <https://reviews.llvm.org/D143670#4116858>, @erichkeane wrote: > The guidance from EWG this week and in the past was that we are always > required to 'parse and diagnose appertainment' of standard attributes, but > not to enable __has_cpp_attribute unless we actually 'do' something with it. > I intend/suggest we add a condition to the CXX tag of 'is supported' with > some sort of conditional for checking diagnostic and O level (or just > straight 'false' in this case). I'm aware of EWG's guidance, and EWG (and WG21) are aware of Clang's position regarding conformance in this area. It's unfortunate that EWG didn't consider implementer feedback or sibling committee feedback to be sufficiently compelling, but at the end of the day, we are the implementers not EWG, and we need to do what's best for us. This implementation is conforming to what the standard says. We're obligated to issue a diagnostic for the situations you identified and we do -- "attribute ignored" is a diagnostic that conveys exactly what we intend it to convey. Maybe someday we will improve conformance in this area but it is explicitly not a priority given that we're still trying to work on C++20 features three years after that standard was released. I suspect we'll get around to this particular "issue" roughly never (given the work, risks, and benefits) which is why WG21 was told this was a burden for us and that we would be conforming in this manner. To be clear about this change specifically -- this is a consistency change. Our existing policy has always been that we do not silently ignore attributes unless they're actively harmful to diagnose as being ignored. `[[carries_dependency]]` isn't even used in practice, so it does not fall into the "harmful to diagnose" category. To date, we've silently accepted it without any intention of implementing the performance improvements it suggests are possible, and that's against our usual policy. What's more, this makes our behavior more self-consistent -- we already do syntactic ignorability of other standard attributes, like `[[no_unique_address]]` on some targets. As we saw when we switched our default mode to C++17, compile time overhead is only getting worse with every release of C++. We do not need to keep the (admittedly small) compile time overhead on every C++ compilation to check for requirements of an attribute we will then ignore entirely. "You're using this wrong but we do nothing with it" is user-hostile behavior for attributes which are inherently non-portable (even the standard ones). I don't expect this to have a measurable impact on compile time overhead in practice, but it's hard to argue that there's value in testing every function parameter of every function declaration to see if it perhaps is using `[[carries_dependency]]` wrong. However, for future attributes like `[[assume]]`, the expense of syntactic checking the argument expression is significant and nontrivial, and we should be self-consistent. (I expect we'll be one of the later C++ implementations to adopt that attribute given it cannot be *semantically* ignored due to potential for template instantiations, changing ABI, etc. We were bitten by WG21 adopting `[[no_unique_address]]` for C++20 and `[[assume]]` has already had at least one implementer push back on implementing it, so there's a very real chance we'll be in the same boat again.) There's ample evidence that total ignorability is not unexpected behavior in practice, either -- the idiomatic way of using attributes is to use feature testing macros to provide various levels of fallback behavior. Note how the fallback clauses do not provide any syntactic checking for the attribute arguments and are defined in a way that semantic appertainment issues are silently ignored as well: https://github.com/mozilla/gecko-dev/blob/master/third_party/highway/hwy/base.h#L159 https://github.com/pytorch/pytorch/blob/master/c10/util/Deprecated.h#L22 https://github.com/boostorg/config/blob/master/include/boost/config/detail/suffix.hpp#L701 Mozilla, pytorch, and boost are not doing anything unusual here and they are all examples of quite popular C++ projects; it seems to be rare that fallback code ever checks that the syntax of the argument is valid or that it’s being applied to the correct thing. Given that these changes are conforming, make us more self-consistent with other C++ standard attributes we don't implement, and makes it less likely to introduce header incompatibilities with C (which has reaffirmed the need for syntactic ignorability of attribute for more than one of their implementations as well), I think we should move forward with these changes to `[[carries_dependency]]` regardless of EWG's guidance. We're following what we're obligated to follow for the standard requirements and WG21 is aware of this. Effectively, I believe our policy should be "follow the C rules for attributes and ignorability" because that does what we need at the moment; we can always revisit the decision later if we find we have the bandwidth. Specific to this review, does anyone have evidence that we have actual users of `[[carries_dependency]]` who will be materially impacted by this change? ================ Comment at: clang/test/Preprocessor/has_attribute.cpp:51 // FIXME(201806L) CHECK: assert: 0 -// CHECK: carries_dependency: 200809L +// CHECK: carries_dependency: 0 // CHECK: deprecated: 201309L ---------------- hubert.reinterpretcast wrote: > I'd trust this when I see the change to [cpp.cond]/6 arrive in CWG. This is consistent with how we handle `[[no_unique_address]]` (see line 59). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143670/new/ https://reviews.llvm.org/D143670 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
