[cfe-dev] Feature request: Don't warn for specified "unknown" attribute

Wed Apr 17 09:26:15 PDT 2019

On Wed, Apr 17, 2019 at 11:52 AM David Blaikie <dblaikie at gmail.com> wrote:
>
> On Wed, Apr 17, 2019 at 8:47 AM Aaron Ballman <aaron at aaronballman.com> wrote:
> >
> > On Wed, Apr 17, 2019 at 10:53 AM David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > > Rather than having to list all the attributes from other compilers you
> > > choose to use - would it make sense to use the namespace/scoping rules
> > > here? Diagnose any unknown attributes that are either unscoped or in
> > > the clang:: scope, but ignore all others?
> >
> > IMO, no. Vendors are welcome to support attributes from other vendors
> > -- for instance, Clang supports some but not nearly all of the
> > attributes in the gnu namespace, so there would continue to be a
> > granularity issue. As for listing attributes individually, there are
> > other design questions that I think don't have clear answers: how to
> > handle attributes with multiple spellings and how to handle attributes
> > that are unknown to one target but not others. We could probably
> > figure something out for those, but I do not think listing out
> > attributes in a warning flag is a tenable option for this
> > functionality. As mentioned, there is already a standards-based
> > approach for solving this problem, which is to use __has_cpp_attribute
> > (and similar macros) and the preprocessor.
>
> Perhaps, but I appreciate Justin's point - there seems limited value
> in the syntax for attributes being standardized if there's no nice way
> to use them without protecting them behind macros anyway. Not zero
> value, but it does reduce the value, I think.

I don't agree that this limits the value of the syntax -- it's a
common idiom that's well-established practice and has been for
numerous years.

That's not to say we shouldn't explore improving it, but I'm not
convinced pushing this into the build system is a good design in
practice. We support around 300 attributes currently, and this is a
fraction of the total attributes supported by all vendors and our list
of supported attributes (as well as other compiler's similar lists)
change with every release. I haven't seen any evidence that a list of
specific attributes to not diagnose is useful in the face of the sheer
number of possible attributes.

> If we support attributes from other namespaces, then including those
> in the set of accepted (& thus warned on) namespaces seems OK to me.
> If someone's using clang and writes gnu::unsupported when they meant
> gnu::supported (even though gnu::unsupported is supported by some
> version of GCC but isn't supported by Clang) that seems like a nice
> thing to warn on in this mode, while silently ignoring
> "unsupported::anything" still.

I would be comfortable having two separate "unknown attribute" warning
flags: -Wunknown-attributes and -Wunknown-attribute-namespaces (or
something) where the distinction is the former warns on any unknown
attribute regardless of namespace, and the latter only diagnoses
unknown attributes in namespaces for which Clang does not support any
attributes. This seems like it would have a useful level of
granularity while not requiring the user to maintain a separate list
of things in the build system. It also nicely sidesteps problems like
attributes with multiple spellings or target-specific attributes.

~Aaron

>
> >
> > ~Aaron
> >
> > >
> > > On Tue, Apr 16, 2019 at 9:03 PM Justin Bassett via cfe-dev
> > > <cfe-dev at lists.llvm.org> wrote:
> > > >>
> > > >> I disagree that users should avoid the preprocessor here. If their
> > > >> code is only compiled by Clang, there is not a lot of compelling
> > > >> reason for them to pass -Wunknown-attributes=whatever in the first
> > > >> place. If the code is compiled by more than just Clang, then the only
> > > >> nonfragile cross-compiler solutions are to use the preprocessor or the
> > > >> build system. In my experience, developers prefer writing code to
> > > >> writing build scripts.
> > > >
> > > >
> > > > Why isn't there a compelling reason to pass -Wunknown-attributes=whatever? Attributes are useful for all sorts of things, such as static analysis tools. And adding a flag to a build script is not "writing build scripts". It's the simplest of build-system logic that I'd expect any developer to know how to do, but not every developer needs to know how to do it, just the ones who add tools to the workflow or update standards, etc.
> > > >
> > > >> I'm not certain why you believe using the preprocessor blocks using a
> > > >> C++11 attribute -- it doesn't. There's no difference between using
> > > >> [[clang::foobar]] and a macro that expands to [[clang::foobar]] if the
> > > >> attribute is supported and [[]] if it's not, except for the lack of a
> > > >> warning about ignored attributes.
> > > >
> > > >
> > > > My point is this: if we use macros anyway, there was no need to standardize attributes in C++11. __attribute__((whatever)) and MSVC's __decspec(...) is sufficient (custom attributes could be handled in a similar way, such as how Qt Moc does it). Yes, things like [[noreturn]] and [[nodiscard]] would need to be wrapped in macros, but that's not too much work.
> > > >
> > > > However, we standardized an attribute syntax for C++11, and in C++17 we clarified that this attribute syntax needs to allow arbitrary attributes, not just standardized or compiler-specific ones.
> > > >
> > > >> I am not convinced that hacking build scripts is less work than using
> > > >> macros. It's arguable that the first time you need the macro is an
> > > >> outsized amount of work (because you have to start adding boilerplate
> > > >> at that point), but I believe the same is true the first time you need
> > > >> to hack a build script to add the new warning flag + attribute list.
> > > >
> > > >
> > > > True. However, macros obscure the code:
> > > >
> > > > SOME_ATTRIBUTE
> > > > void call_me();
> > > >
> > > > It may be clear that SOME_ATTRIBUTE is an attribute, but I have to chase down the definition of SOME_ATTRIBUTE to determine what it is.
> > > >
> > > > Also consider that some users will prefer to pass -Wno-unknown-attributes than use these macros, meaning they get no typo detection. However, if they could whitelist allowed attributes to turn off the warning for, I'd imagine many of those users would do so. Why not have a fine-grained option for warning control?
> > > >
> > > >> Given that (AFAIK) no other compiler gives you granularity to disable
> > > >> unknown attribute warnings on a per-attribute basis, I don't think
> > > >> forcing users to try to deal with this in build scripts does users a
> > > >> service.
> > > >
> > > >
> > > > Just because no compiler does it yet doesn't mean we shouldn't do it. I've submitted a similar feature request on GCC, and it seems well-received. I intend to do the same for MSVC.
> > > >
> > > >> This is especially true given that each compiler will require
> > > >> a different build script change even if the flags are identical
> > > >> between compilers, because different compilers support different lists
> > > >> of attributes.
> > > >
> > > >
> > > > If different compilers support different lists of attributes, that's not a problem. A -Wignore-unknown-attribute=clang::some_attribute in GCC would be fine in Clang, because it does nothing in Clang. The flag may differ between compilers, though, but that's not anything developers aren't already familiar with.
> > > >
> > > > --------
> > > >
> > > > I asked the Cpplang Slack how they felt about this: https://cpplang.slack.com/archives/C21PKDHSL/p1555422655443800 . I'm clearly not alone in thinking this idea is reasonable.
> > > >
> > > >
> > > > Yes, macros are currently a solution to the problem. But that doesn't mean that a better solution doesn't exist. I don't agree that "add more macros" is a good solution, but "turn off the warnings in a focused way" is a bad solution.
> > > > If ignoring specific attributes is a lot of work or a pain because of having to manually maintain the list in the build system, there are several alternatives, such as only warning for unknown unnamespaced (standard) attributes, [[gnu::attribute]]s, and [[clang::attribute]]s. Any other namespace would be ignored.
> > > >
> > > > Regards,
> > > > Justin
> > > >
> > > > _______________________________________________
> > > > cfe-dev mailing list
> > > > cfe-dev at lists.llvm.org
> > > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev