Re: [libcxx] r291961 - Add _LIBCPP_DIAGNOSE_WARNING and _LIBCPP_DIAGNOSE_ERROR macros.
The only plan that we have at the moment is basically for a
-Wno-user-defined-warnings-in-system-headers type of flag. I agree that it
would be nice if we could be more granular than this, so I'll think about
what we can do.
On Mon, Jan 23, 2017 at 8:36 AM, Nico Weber wrote:
> This happens to fire in practice in protobuf. It's probably a true
> positive and it's cool that this warning found it, but it means we have to
> disable Wuser-defined-warnings for a bit -- which then disables all of
> these user-defined warnings. Right now there aren't any others, but it
> feels like we'd want to have the ability to turn individual user-defined
> warnings on or off instead of just having a single toggle for all of them.
> Are there plans for something like that?
>
> On Fri, Jan 13, 2017 at 5:02 PM, Eric Fiselier via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: ericwf
>> Date: Fri Jan 13 16:02:08 2017
>> New Revision: 291961
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=291961&view=rev
>> Log:
>> Add _LIBCPP_DIAGNOSE_WARNING and _LIBCPP_DIAGNOSE_ERROR macros.
>>
>> Clang recently added a `diagnose_if(cond, msg, type)` attribute
>> which can be used to generate diagnostics when `cond` is a constant
>> expression that evaluates to true. Otherwise no attribute has no
>> effect.
>>
>> This patch adds _LIBCPP_DIAGNOSE_ERROR/WARNING macros which
>> use this new attribute. Additionally this patch implements
>> a diagnostic message when a non-const-callable comparator is
>> given to a container.
>>
>> Note: For now the warning version of the diagnostic is useless
>> within libc++ since warning diagnostics are suppressed by the
>> system header pragma. I'm going to work on fixing this.
>>
>> Added:
>> libcxx/trunk/test/libcxx/containers/associative/non_const_co
>> mparator.fail.cpp
>> Modified:
>> libcxx/trunk/docs/UsingLibcxx.rst
>> libcxx/trunk/include/__config
>> libcxx/trunk/include/__tree
>> libcxx/trunk/include/map
>> libcxx/trunk/include/type_traits
>> libcxx/trunk/test/libcxx/compiler.py
>> libcxx/trunk/test/libcxx/test/config.py
>> libcxx/trunk/test/libcxx/test/format.py
>> libcxx/trunk/test/libcxx/utilities/tuple/tuple.tuple/diagnos
>> e_reference_binding.fail.cpp
>>
>> Modified: libcxx/trunk/docs/UsingLibcxx.rst
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/UsingL
>> ibcxx.rst?rev=291961&r1=291960&r2=291961&view=diff
>>
>> ==
>> --- libcxx/trunk/docs/UsingLibcxx.rst (original)
>> +++ libcxx/trunk/docs/UsingLibcxx.rst Fri Jan 13 16:02:08 2017
>> @@ -173,3 +173,10 @@ thread safety annotations.
>>return Tup{"hello world", 42}; // explicit constructor called. OK.
>> }
>>
>> +**_LIBCPP_DISABLE_ADDITIONAL_DIAGNOSTICS**:
>> + This macro disables the additional diagnostics generated by libc++
>> using the
>> + `diagnose_if` attribute. These additional diagnostics include checks
>> for:
>> +
>> +* Giving `set`, `map`, `multiset`, `multimap` a comparator which is
>> not
>> + const callable.
>> +
>>
>> Modified: libcxx/trunk/include/__config
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__c
>> onfig?rev=291961&r1=291960&r2=291961&view=diff
>>
>> ==
>> --- libcxx/trunk/include/__config (original)
>> +++ libcxx/trunk/include/__config Fri Jan 13 16:02:08 2017
>> @@ -1006,6 +1006,16 @@ _LIBCPP_FUNC_VIS extern "C" void __sanit
>> #endif
>> #endif
>>
>> +#if __has_attribute(diagnose_if) && !defined(_LIBCPP_DISABLE_ADDIT
>> IONAL_DIAGNOSTICS)
>> +# define _LIBCPP_DIAGNOSE_WARNING(...) \
>> +__attribute__((__diagnose_if__(__VA_ARGS__, "warning")))
>> +# define _LIBCPP_DIAGNOSE_ERROR(...) \
>> +__attribute__((__diagnose_if__(__VA_ARGS__, "error")))
>> +#else
>> +# define _LIBCPP_DIAGNOSE_WARNING(...)
>> +# define _LIBCPP_DIAGNOSE_ERROR(...)
>> +#endif
>> +
>> #endif // __cplusplus
>>
>> #endif // _LIBCPP_CONFIG
>>
>> Modified: libcxx/trunk/include/__tree
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__t
>> ree?rev=291961&r1=291960&r2=291961&view=diff
>>
>> ==
>> --- libcxx/trunk/include/__tree (original)
>> +++ libcxx/trunk/include/__tree Fri Jan 13 16:02:08 2017
>> @@ -41,6 +41,10 @@ template
>> struct __value_type;
>> #endif
>>
>> +template > +bool = is_empty<_Compare>::value && !__libcpp_is_final<_Compare>::
>> value>
>> +class __map_value_compare;
>> +
>> template class __map_node_destructor;
>> template class _LIBCPP_TEMPLATE_VIS __map_iterator;
>> template class _LIBCPP_TEMPLATE_VIS
>> __map_const_iterator;
>> @@ -955,6 +959,30 @@ private:
>>
>> };
>>
>> +#ifndef _LIBCPP_CXX03_LANG
>> +template
>> +struct __diagnose_tree_helper {
>> + static constexpr bool __trigger_diagnostics()
>> + _LIBCPP_DIAGNOSE_WARNING(!__is_const_compar
Re: [libcxx] r291961 - Add _LIBCPP_DIAGNOSE_WARNING and _LIBCPP_DIAGNOSE_ERROR macros.
I like the idea of tagging diagnose_ifs with tags, though I wonder how this
could be made applicable to code outside of libcxx. Specifically, if I
import some big library that uses diagnose_if, then I'd still need to use
`-Wno-user-defined-warnings` if said lib had a single diagnose_if that I
didn't want to fix immediately.
My thought is that we may be able to tag these IDs with namespaces, and we
can match against those in the -Wno-user-defined-warnings=whatever.
For example:
namespace std {
void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
"foo_tag")));
void bar(int i) __attribute__((diagnose_if(i, "oh no", "warning",
"bar_tag")));
}
-Wno-user-defined-warnings='std::' // all diagnose_ifs in std:: are disabled
-Wno-user-defined-warnings='std::foo_tag' // std::bar can warn
-Wno-user-defined-warnings='std::foo_tag,std::bar_tag' // neither std::foo
nor std::bar warn
If not this, then I'd like to at least consider some kind of wildcarding
thing in the warning flag, since libraries will likely roll their own
tag-namespaces anyway to avoid things like:
namespace boost {
void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
"foo")));
}
namespace internal_lib {
void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
"foo")));
}
-Wno-user-defined-warnings='foo' // disables boost::foo and
internal_lib::foo's warnings.
To be clear, I'm imagining the wildcards would look something like
#define _LIBCXX_TAG "libcxx_warning_"
namespace std {
void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
_LIBCXX_TAG "foo_tag")));
void bar(int i) __attribute__((diagnose_if(i, "oh no", "warning",
_LIBCXX_TAG "bar_tag")));
}
-Wno-user-defined-warnings='libcxx_warning*' // std::foo and std::bar
warnings are disabled
-Wno-user-defined-warnings='libcxx_warning_foo_tag' // std::bar can warn
-Wno-user-defined-warnings='libcxx_warning_foo_tag,libcxx_warning_bar_tag'
// neither std::foo nor std::bar warn
...Personally, I'm leaning toward the implicit namespacing, since it's
impossible to forget (e.g. a library that doesn't use tags can be disabled
with 'lib_namespace::', instead of having to disable all user-defined
warnings), but I'd be content with either.
On Mon, Jan 23, 2017 at 2:11 PM, Eric Fiselier via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> My dream, and something I would like to work towards is supporting
> something like this:
>
>
On Mon, Jan 23, 2017 at 2:11 PM, Eric Fiselier wrote:
> My dream, and something I would like to work towards is supporting
> something like this:
>
> > [[clang::libcxx_diagnose_if(cond, "message", "warning", /* warning-id*/
> "non-const-functor")]]
> >
> > -Wno-libcxx-warnings=non-const-functor
>
> This way libc++ warnings get treated differently from all other users of
> diagnose_if, and can be enabled
> and disabled separately.
>
> @George does this sound like a reasonable goal? If so I'm happy to start
> working on this.
>
> /Eric
>
>
> On Mon, Jan 23, 2017 at 11:46 AM, George Burgess wrote:
>
>> The only plan that we have at the moment is basically for a
>> -Wno-user-defined-warnings-in-system-headers type of flag. I agree that
>> it would be nice if we could be more granular than this, so I'll think
>> about what we can do.
>>
>> On Mon, Jan 23, 2017 at 8:36 AM, Nico Weber wrote:
>>
>>> This happens to fire in practice in protobuf. It's probably a true
>>> positive and it's cool that this warning found it, but it means we have to
>>> disable Wuser-defined-warnings for a bit -- which then disables all of
>>> these user-defined warnings. Right now there aren't any others, but it
>>> feels like we'd want to have the ability to turn individual user-defined
>>> warnings on or off instead of just having a single toggle for all of them.
>>> Are there plans for something like that?
>>>
>>> On Fri, Jan 13, 2017 at 5:02 PM, Eric Fiselier via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
Author: ericwf
Date: Fri Jan 13 16:02:08 2017
New Revision: 291961
URL: http://llvm.org/viewvc/llvm-project?rev=291961&view=rev
Log:
Add _LIBCPP_DIAGNOSE_WARNING and _LIBCPP_DIAGNOSE_ERROR macros.
Clang recently added a `diagnose_if(cond, msg, type)` attribute
which can be used to generate diagnostics when `cond` is a constant
expression that evaluates to true. Otherwise no attribute has no
effect.
This patch adds _LIBCPP_DIAGNOSE_ERROR/WARNING macros which
use this new attribute. Additionally this patch implements
a diagnostic message when a non-const-callable comparator is
given to a container.
Note: For now the warning version of the diagnostic is useless
within libc++ since warning diagnostics are suppressed by the
system header pragma. I'm going to work on fixing this.
Added:
libcxx/trunk/test/libcxx/containers/associative/non_const_co
mparator.fail.cpp
Modified:
libcxx/trunk/docs/U
Re: [libcxx] r291961 - Add _LIBCPP_DIAGNOSE_WARNING and _LIBCPP_DIAGNOSE_ERROR macros.
> However I do want a little special behavior for libc++ [...]
I have no problem with this: I just wanted to be sure that we could
reasonably use this feature for things that aren't libcxx. :)
> Libc++ defines some symbols within the global namespace, and we'll need a
way to group those as part of libc++ as well.
Good point. It also seems that implicitly namespacing things would be
problematic for things like specializing std::hash (and might get fun when
mixed with inline namespaces?) so I'm liking the wildcard +
please-just-add-a-prefix-to-your-IDs strategy more now.
On Mon, Jan 23, 2017 at 3:35 PM, Eric Fiselier wrote:
> > I like the idea of tagging diagnose_ifs with tags, though I wonder how
> this could be made applicable to code outside of libcxx. Specifically, if I
> import some big library that uses diagnose_if, then I'd still need to use
> `-Wno-user-defined-warnings` if said lib had a single diagnose_if that I
> didn't want to fix immediately.
>
> Yeah, we absolutely want this code to be applicable outside of libc++,
> other libraries and their users will have the same need to
> tag/enable/disable warnings separately.
> However I do want a little special behavior for libc++. Specifically I
> dislike that libc++ warnings are part of -Wuser-defined-warnings, since it
> should be considered part
> of the C++ implementation, and not part of "user-code". Instead I would
> greatly appreciate if libc++ had its own top-level warning group
> -Wstandard-library-warnings, since
> many of the diagnostics it generates will be about violations of the C++
> standard. Also when other libraries start using diagnose_if I don't want
> the libc++ warnings
> getting suppressed when a user adds -Wno-user-defined-warnings to disable
> diagnostics produced elsewhere.
>
> I'm not sure what the best way to achieve this, but that's my rational for
> wanting it.
>
> > If not this, then I'd like to at least consider some kind of
> wildcarding thing in the warning flag, since libraries will likely roll
> their own tag-namespaces anyway to avoid things like:
>
> Libc++ defines some symbols within the global namespace, and we'll need a
> way to group those as part of libc++ as well.
>
> /Eric
>
>
>
>
> On Mon, Jan 23, 2017 at 4:06 PM, George Burgess wrote:
>
>> I like the idea of tagging diagnose_ifs with tags, though I wonder how
>> this could be made applicable to code outside of libcxx. Specifically, if I
>> import some big library that uses diagnose_if, then I'd still need to use
>> `-Wno-user-defined-warnings` if said lib had a single diagnose_if that I
>> didn't want to fix immediately.
>>
>> My thought is that we may be able to tag these IDs with namespaces, and
>> we can match against those in the -Wno-user-defined-warnings=whatever.
>>
>> For example:
>>
>> namespace std {
>> void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
>> "foo_tag")));
>> void bar(int i) __attribute__((diagnose_if(i, "oh no", "warning",
>> "bar_tag")));
>> }
>>
>> -Wno-user-defined-warnings='std::' // all diagnose_ifs in std:: are
>> disabled
>> -Wno-user-defined-warnings='std::foo_tag' // std::bar can warn
>> -Wno-user-defined-warnings='std::foo_tag,std::bar_tag' // neither
>> std::foo nor std::bar warn
>>
>> If not this, then I'd like to at least consider some kind of wildcarding
>> thing in the warning flag, since libraries will likely roll their own
>> tag-namespaces anyway to avoid things like:
>>
>> namespace boost {
>> void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
>> "foo")));
>> }
>> namespace internal_lib {
>> void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
>> "foo")));
>> }
>>
>> -Wno-user-defined-warnings='foo' // disables boost::foo and
>> internal_lib::foo's warnings.
>>
>> To be clear, I'm imagining the wildcards would look something like
>>
>> #define _LIBCXX_TAG "libcxx_warning_"
>> namespace std {
>> void foo(int i) __attribute__((diagnose_if(i, "oh no", "warning",
>> _LIBCXX_TAG "foo_tag")));
>> void bar(int i) __attribute__((diagnose_if(i, "oh no", "warning",
>> _LIBCXX_TAG "bar_tag")));
>> }
>>
>> -Wno-user-defined-warnings='libcxx_warning*' // std::foo and std::bar
>> warnings are disabled
>> -Wno-user-defined-warnings='libcxx_warning_foo_tag' // std::bar can warn
>> -Wno-user-defined-warnings='libcxx_warning_foo_tag,libcxx_warning_bar_tag'
>> // neither std::foo nor std::bar warn
>>
>> ...Personally, I'm leaning toward the implicit namespacing, since it's
>> impossible to forget (e.g. a library that doesn't use tags can be disabled
>> with 'lib_namespace::', instead of having to disable all user-defined
>> warnings), but I'd be content with either.
>>
>> On Mon, Jan 23, 2017 at 2:11 PM, Eric Fiselier via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> My dream, and something I would like to work towards is supporting
>>> something like this:
>>>
>>>
>> On Mon, Jan 23, 2017 at 2:11 PM, Eric Fiselier wrote:
>>
>>> My dream, and something I wo
Re: [PATCH] __attribute__((enable_if)) and non-overloaded member functions
Sure. :) Review is based off the attachment I grabbed from here:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150824/136904.html
A few nits:
- test/Sema/enable_if.cpp line 24: Please use __attribute__(( instead of
__attribute((
- Can we have a similar test for a function that returns an Incomplete?
George
On Wed, Sep 16, 2015 at 1:30 PM, Nick Lewycky wrote:
> +gbiv, would you be able to review this?
> On Sep 14, 2015 7:42 AM, "Ettore Speziale"
> wrote:
>
>> Ping
>>
>> > Gently ping.
>> >
>> >> On Aug 26, 2015, at 2:40 PM, Ettore Speziale <
>> speziale.ett...@gmail.com> wrote:
>> >>
>> >> Forward to the right ML:
>> >>
>> Sorry about the extreme delay. This patch slipped through the
>> cracks, and I only noticed it again when searching my email for enable_if.
>> Committed in r245985! In the future, please feel free to continue pinging
>> weekly!
>> >>>
>> >>> NP, thank you for committing the patch.
>> >>>
>> >>> Unfortunately it contains a little error in the case of no candidate
>> has been found. For instance consider the following test case:
>> >>>
>> >>> struct Incomplete;
>> >>>
>> >>> struct X {
>> >>> void hidden_by_argument_conversion(Incomplete n, int m = 0)
>> __attribute((enable_if(m == 10, "chosen when 'm' is ten")));
>> >>> };
>> >>>
>> >>> x.hidden_by_argument_conversion(10);
>> >>>
>> >>> I would expect to get an error about Incomplete, as the compiler
>> cannot understand how to convert 10 into an instance of Incomplete. However
>> right now the enable_if diagnostic is emitted, thus masking the more useful
>> message about Incomplete.
>> >>>
>> >>> The attached patch solved the problem by delaying the point where the
>> enable_if diagnostic is issued.
>> >>>
>> >>> Thanks,
>> >>> Ettore Speziale
>> >>
>> >>
>> >>
>> >
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] __attribute__((enable_if)) and non-overloaded member functions
> sorry for the late reply, I did not note this email … No problem! :) It looks like the attached patch is the same as the original one? George On Mon, Sep 21, 2015 at 10:31 AM, Ettore Speziale wrote: > Hello, > > sorry for the late reply, I did not note this email … > > > > Sure. :) Review is based off the attachment I grabbed from here: > http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150824/136904.html > > That’s correct. > > > A few nits: > > - test/Sema/enable_if.cpp line 24: Please use __attribute__(( instead of > __attribute(( > > - Can we have a similar test for a function that returns an Incomplete? > > Sure. Also in that case, only the error about Incomplete is reported. > Please check the attached patch. > > > > > Thank you very much, > Ettore Speziale > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] __attribute__((enable_if)) and non-overloaded member functions
Looks good to me! Do you have commit access, or would you like for me to commit it for you? George On Tue, Sep 22, 2015 at 7:45 AM, Ettore Speziale wrote: > Hello, > > > It looks like the attached patch is the same as the original one? > > I’ve attache the wrong patch. Here is the right one: > > > > Thanks, > Ettore Speziale > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] __attribute__((enable_if)) and non-overloaded member functions
Sorry for the delay -- didn't see this email come in. Committed as r248595. :) On Tue, Sep 22, 2015 at 1:06 PM, Ettore Speziale wrote: > Hello, > > > Looks good to me! > > > > Do you have commit access, or would you like for me to commit it for you? > > I do not have commit access. It would be great if you can commit for me. > > Thanks, > Ettore Speziale > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
