cpp-sort icon indicating copy to clipboard operation
cpp-sort copied to clipboard

Error messages are unreadable

Open Morwenn opened this issue 10 years ago • 8 comments

In its current state, the library produces rather unreadable error messages and can spawn several hundreds of lines of cryptic error messages for a simple missing const without a single hint to what may be wrong. Honestly, this is a major drawback and the crazy amount of SFINAE insanity doesn't help.

We can't realistically put static assertions all over the place because many mechanisms in the library rely on SFINAE. There may be other ways to handle errors.


Some elements of answer:

  • [x] Use strong typedefs via inheritance instead of templates aliases to define sorters.
  • [ ] Concepts will probably help to reduce the error madness, but we have to wait for C++20 at least.
  • [x] If the DR allowing to write using Sorters::operator()...; makes its way to C++17, it might remove some annoying errors dues to recursion.
  • [x] Static assertions could be used after in the sorters' operator() to ensure that the iterator category of the parameter is correct. By the time the operator is called, all of the SFINAE checks should already have passed and the only remaining thing before the actual sort is an eventual tag dispatch. Unfortunately, I doubt that it is possible to automate this mechanism.
  • [ ] Give a default template parameter to hybrid_adapter which would act as a fallback if none of the sorters are picked and trigger an explicit error. Getting the mechanism to work with nested hybrid_adapter seems crazy hard though.
  • [x] Update the tutorials to stress out the importance of errors to potential sorter writers.
  • [x] ~~More static assertions for sorters that only accept copyable types. The error messages when trying to feed a collection of move-only types to a sorter that needs to copy them are not really readable~~ (solved by #15, all sorters now work with move-only types).
  • [x] ~~Moving the SFINAE condition of type-specific sorters to a dedicated trait may allow to add more static_assert to the sorter in the simplest case in order to reduce the amount of SFINAE logs (see #113).~~

To be honest the errors occuring when a const is missing are the worst kind. There might be ways to require less const everywhere while still providing it when needed.


A few resources about getting better error messages:

Morwenn avatar Nov 18 '15 14:11 Morwenn

(unreadable*. here, did that help?)

vendethiel avatar Nov 18 '15 16:11 vendethiel

@vendethiel Haha, it was more along the lines of « we want readable error messages », but you're right, I will change the title :p

Morwenn avatar Nov 18 '15 16:11 Morwenn

:D sorry, I'm terrible at C++. Just ask my coworkers ;)

vendethiel avatar Nov 18 '15 17:11 vendethiel

Apparently, some tricks exist to display better error messages upon SFINAE failure. I can probably check whether some of them could be used to improve cpp-sort's error messages too.

Morwenn avatar May 19 '16 22:05 Morwenn

have you looked at the sqlpp guy's talk?

vendethiel avatar May 20 '16 11:05 vendethiel

Yes I have (well, I have read the slides). The « strong typedef via inheritance » as well as the mention of concepts come from here. It's even possible that I opened this issue because of the talk actually.

Morwenn avatar May 20 '16 11:05 Morwenn

that makes sense 👍

vendethiel avatar May 20 '16 11:05 vendethiel

Two small changes that could reduce the compiler call stack in error messages:

  • Replace use of std::distance(it1, it2) and std::advance(it, n) with (it2 - it1) and it += n when the algorithm is known to deal with random-access iterators.
  • Don't use std::begin and std::end when the type of the container is known and unlikely to change.

Those little functions are used everywhere in the code base, don't offer much outside of sufficiently generic contexts and make can make calls stacks grow. Moreover it's easier to track the origin of a problem directly when (it2 - it1) appears directly in the algorithm rather than in the implementation of std::distance.

EDIT: done in commit 5c6d89a37f86edae7ce9172c61cc7f27e8a6f3c6

Morwenn avatar Jun 22 '20 17:06 Morwenn