Page MenuHomePhabricator

[libc++] Improve diagnostics for non-const comparators and hashers in associative containers
ClosedPublic

Authored by ldionne on Jul 4 2018, 2:55 PM.

Details

Summary

When providing a non-const-callable comparator in a map or set, the
warning diagnostic does not include the point of instantiation of
the container that triggered the warning, which makes it difficult
to track down the problem. This commit improves the diagnostic by
placing it directly in the body of the associative container.

The same change is applied to unordered associative containers, which
had a similar problem.

Finally, this commit cleans up the forward declarations of several
map and unordered_map helpers, which are not needed anymore.

rdar://problem/41370747

Event Timeline

ldionne created this revision.Jul 4 2018, 2:55 PM

Before the patch, the backtrace refers to the instantiation of the set's destructor instead of referring to the set's instantiation itself:

BEFORE (ordered associative containers)

In file included from /Users/ldionne/work/llvm/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp:16:
In file included from /Users/ldionne/work/llvm/libcxx/include/set:389:
/Users/ldionne/work/llvm/libcxx/include/__tree:1812:22: warning: the specified comparator type does not provide a const call operator [-Wuser-defined-warnings]
                     __trigger_diagnostics()), "");
                     ^
/Users/ldionne/work/llvm/libcxx/include/set:400:28: note: in instantiation of member function 'std::__1::__tree<int, BadCompare, std::__1::allocator<int> >::~__tree' requested here
class _LIBCPP_TEMPLATE_VIS set
                           ^
/Users/ldionne/work/llvm/libcxx/include/__tree:963:7: note: from 'diagnose_if' attribute on '__trigger_diagnostics':
      _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
      ^                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ldionne/work/llvm/libcxx/include/__config:1165:21: note: expanded from macro '_LIBCPP_DIAGNOSE_WARNING'
     __attribute__((diagnose_if(__VA_ARGS__, "warning")))
                    ^           ~~~~~~~~~~~

AFTER (ordered associative containers)

In file included from /Users/ldionne/work/llvm/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp:16:
/Users/ldionne/work/llvm/libcxx/include/set:412:26: warning: the specified comparator type does not provide a const call operator [-Wuser-defined-warnings]
    static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
                         ^
/Users/ldionne/work/llvm/libcxx/test/libcxx/containers/associative/non_const_comparator.fail.cpp:34:7: note: in instantiation of template class 'std::__1::set<int, BadCompare, std::__1::allocator<int> >' requested here
    C s;
      ^
/Users/ldionne/work/llvm/libcxx/include/__tree:957:5: note: from 'diagnose_if' attribute on '__diagnose_non_const_comparator<int, BadCompare>':
    _LIBCPP_DIAGNOSE_WARNING(!std::__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
    ^                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ldionne/work/llvm/libcxx/include/__config:1165:21: note: expanded from macro '_LIBCPP_DIAGNOSE_WARNING'
     __attribute__((diagnose_if(__VA_ARGS__, "warning")))
                    ^           ~~~~~~~~~~~

For unordered containers, the difference is that we don't show __hash_table in the backtrace anymore:

BEFORE (unordered associative containers)

In file included from /Users/ldionne/work/llvm/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp:16:
In file included from /Users/ldionne/work/llvm/libcxx/include/unordered_set:323:
/Users/ldionne/work/llvm/libcxx/include/__hash_table:956:77: warning: the specified hash functor does not provide a const call operator [-Wuser-defined-warnings]
    static_assert(__diagnose_hash_table_helper<_Tp, _Hash, _Equal, _Alloc>::__trigger_diagnostics(), "");
                                                                            ^
/Users/ldionne/work/llvm/libcxx/include/unordered_set:353:13: note: in instantiation of template class 'std::__1::__hash_table<int, BadHash, BadEqual, std::__1::allocator<int> >' requested here
    __table __table_;
            ^
/Users/ldionne/work/llvm/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp:42:7: note: in instantiation of template class 'std::__1::unordered_set<int, BadHash, BadEqual, std::__1::allocator<int> >' requested here
    C s;
      ^
/Users/ldionne/work/llvm/libcxx/include/__hash_table:867:5: note: from 'diagnose_if' attribute on '__trigger_diagnostics':
    _LIBCPP_DIAGNOSE_WARNING(__check_hash_requirements<_Key, _Hash>::value
    ^                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ldionne/work/llvm/libcxx/include/__config:1165:21: note: expanded from macro '_LIBCPP_DIAGNOSE_WARNING'
     __attribute__((diagnose_if(__VA_ARGS__, "warning")))
                    ^           ~~~~~~~~~~~

AFTER (unordered associative containers)

In file included from /Users/ldionne/work/llvm/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp:16:
/Users/ldionne/work/llvm/libcxx/include/unordered_set:349:26: warning: the specified hash functor does not provide a const call operator [-Wuser-defined-warnings]
    static_assert(sizeof(__diagnose_unordered_container_requirements<_Value, _Hash, _Pred>(0)), "");
                         ^
/Users/ldionne/work/llvm/libcxx/test/libcxx/containers/unord/non_const_comparator.fail.cpp:44:7: note: in instantiation of template class 'std::__1::unordered_set<int, BadHash, BadEqual, std::__1::allocator<int> >' requested here
    C s;
      ^
/Users/ldionne/work/llvm/libcxx/include/__hash_table:868:5: note: from 'diagnose_if' attribute on '__diagnose_unordered_container_requirements<int, BadHash, BadEqual>':
    _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Hash const&, _Key const&>::value,
    ^                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ldionne/work/llvm/libcxx/include/__config:1165:21: note: expanded from macro '_LIBCPP_DIAGNOSE_WARNING'
     __attribute__((diagnose_if(__VA_ARGS__, "warning")))
                    ^           ~~~~~~~~~~~
ldionne updated this revision to Diff 168318.Oct 4 2018, 9:46 AM

Rebase on top of ToT.

If you can build this w/GCC in C++03 mode, then I'm fine with it.

If you can build this w/GCC in C++03 mode, then I'm fine with it.

I can't run the tests with GCC in C++03 mode -- I get thousands of errors unrelated to this patch.

If you can build this w/GCC in C++03 mode, then I'm fine with it.

I can't run the tests with GCC in C++03 mode -- I get thousands of errors unrelated to this patch.

But the two tests I modified do pass, since they are unsupported on GCC. Am I good to go?

I'll push this as I'm fairly sure this won't affect the (already broken) build on GCC. At least not as far as I can tell.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 6 2018, 1:49 PM
This revision was automatically updated to reflect the committed changes.