All asserts need to have messages attached! Explicit as possible!
I came across some error in heap sort. Was erroring out on the
wrong. A few lines below the assert existed but no error message
associated, why is it so hard to not write a few extra EXTREMELY
helpful error messages?
assert(isHeap(r), "This is an ERROR AT THIS
LOCATION"~__FILE__~"("~__LINE__~")");
etc?
It should be mandatory that all asserts, throws, etc provide
correct information about not only the point of the error but
also the location and what caused it. These things are not
irrelevant but affect all those that use it... imagine the real
human man hours that could be saved if such things were done.
It would be easy to find all the bad asserts?
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote: It would be easy to find all the bad asserts? Does Dscanner have a rule to detect assert and enforce calls without a message? We should have it enabled for Phobos.
Re: All asserts need to have messages attached! Explicit as possible!
On 08.07.2017 02:55, FoxyBrown wrote:
I came across some error in heap sort. Was erroring out on the wrong. A
few lines below the assert existed but no error message associated, why
is it so hard to not write a few extra EXTREMELY helpful error messages?
assert(isHeap(r), "This is an ERROR AT THIS
LOCATION"~__FILE__~"("~__LINE__~")");
etc?
It should be mandatory that all asserts, throws, etc provide correct
information about not only the point of the error but also the location
and what caused it. These things are not irrelevant but affect all those
that use it... imagine the real human man hours that could be saved if
such things were done.
It would be easy to find all the bad asserts?
It think the main issue is that the assertion failure had the wrong
location information. This is not supposed to happen. Do you have a
small example that demonstrates the issue?
I think that as long as the location is correct, the rest can be
reconstructed without wasting a lot of time. (Of course, this is no
excuse for Phobos not providing a more informative error message.)
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, 8 July 2017 at 01:01:38 UTC, Vladimir Panteleev wrote: On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote: It would be easy to find all the bad asserts? Does Dscanner have a rule to detect assert and enforce calls without a message? We should have it enabled for Phobos. No, but now there's: https://github.com/dlang-community/D-Scanner/pull/495 I can always add this to the phobos branch at DScanner if you want to test this immediately.
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
[...]
It should be mandatory that all asserts, throws, etc provide
correct information about not only the point of the error but
also the location and what caused it. [...]
Error messages are sensible when they provide context (e.g. for
complex conditions).
In my experience, though, most asserts exist to cover fairly
obvious cases, where an additional message would just add noise:
---
struct SomeRange
{
bool empty() { ... }
void popFront()
{
assert (!empty);
...
}
}
---
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, July 8, 2017 1:51:34 AM MDT Moritz Maxeiner via Digitalmars-d
wrote:
> On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
> > [...]
> > It should be mandatory that all asserts, throws, etc provide
> > correct information about not only the point of the error but
> > also the location and what caused it. [...]
>
> Error messages are sensible when they provide context (e.g. for
> complex conditions).
> In my experience, though, most asserts exist to cover fairly
> obvious cases, where an additional message would just add noise:
>
> ---
> struct SomeRange
> {
> bool empty() { ... }
> void popFront()
> {
> assert (!empty);
> ...
> }
> }
> ---
I tend to agree - in many cases, the error message is completely redundant -
but a number of folks seem to think that you should never have an assertion
without a message, and it tends to get complained about if you have an
assertion without a message if it's not in a unit test, so I'm a bit
surprised that the OP ran into one in Phobos.
- Jonathan M Davis
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, July 8, 2017 12:55:46 AM MDT FoxyBrown via Digitalmars-d wrote:
> I came across some error in heap sort. Was erroring out on the
> wrong. A few lines below the assert existed but no error message
> associated, why is it so hard to not write a few extra EXTREMELY
> helpful error messages?
>
> assert(isHeap(r), "This is an ERROR AT THIS
> LOCATION"~__FILE__~"("~__LINE__~")");
>
> etc?
>
> It should be mandatory that all asserts, throws, etc provide
> correct information about not only the point of the error but
> also the location and what caused it. These things are not
> irrelevant but affect all those that use it... imagine the real
> human man hours that could be saved if such things were done.
>
> It would be easy to find all the bad asserts?
AssertError already provides the location of the assertion, and it uses
__FILE__ and __LINE__ to do it (_all_ Exception types do that unless the
person who wrote the class screwed up the constructor), so adding them to
the message it pointless and redundant. If the assertion failure is printing
out the wrong line, then it's likely either because you're looking at the
wrong version of the code or because a string mixin is screwing with the
line numbers (which IMHO, shouldn't happen, but it at least used to be a
problem).
In addition, the AssertError should be giving you a stack trace, which
_should_ have the file and line numbers in it of every call in the stack
(though stupidly, the line numbers don't always work, depending on your OS).
So, you _should_ have had that information by the simple fact that an
AssertError was thrown, and unless the assertion itself needed additional
explanation beyond the condition that failed, then a message wouldn't have
helped anyway.
So, if the file and line number was wrong, the question is why, and that
should be fixed. And we really need to figure out how to make it so that the
issue of not having line numbers with stack traces goes away and stays away.
The fact that it's been a problem is ridiculous, because file and line
numvbers in stack traces are critical for debugging.
- Jonathan M Davis
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, 8 July 2017 at 02:23:53 UTC, Jonathan M Davis wrote:
On Saturday, July 8, 2017 12:55:46 AM MDT FoxyBrown via
Digitalmars-d wrote:
I came across some error in heap sort. Was erroring out on the
wrong. A few lines below the assert existed but no error
message associated, why is it so hard to not write a few extra
EXTREMELY helpful error messages?
assert(isHeap(r), "This is an ERROR AT THIS
LOCATION"~__FILE__~"("~__LINE__~")");
etc?
It should be mandatory that all asserts, throws, etc provide
correct information about not only the point of the error but
also the location and what caused it. These things are not
irrelevant but affect all those that use it... imagine the
real human man hours that could be saved if such things were
done.
It would be easy to find all the bad asserts?
AssertError already provides the location of the assertion, and
it uses __FILE__ and __LINE__ to do it (_all_ Exception types
do that unless the person who wrote the class screwed up the
constructor), so adding them to the message it pointless and
redundant. If the assertion failure is printing out the wrong
line, then it's likely either because you're looking at the
wrong version of the code or because a string mixin is screwing
with the line numbers (which IMHO, shouldn't happen, but it at
least used to be a problem).
In addition, the AssertError should be giving you a stack
trace, which _should_ have the file and line numbers in it of
every call in the stack (though stupidly, the line numbers
don't always work, depending on your OS).
So, you _should_ have had that information by the simple fact
that an AssertError was thrown, and unless the assertion itself
needed additional explanation beyond the condition that failed,
then a message wouldn't have helped anyway.
So, if the file and line number was wrong, the question is why,
and that should be fixed. And we really need to figure out how
to make it so that the issue of not having line numbers with
stack traces goes away and stays away. The fact that it's been
a problem is ridiculous, because file and line numvbers in
stack traces are critical for debugging.
- Jonathan M Davis
But you miss the complete point, You focused on what D does
rather than what it doesn't, which is what lead me to the problem
in the first place. You are seeing the glass half full, rather
than half empty, my friend.
Shall I explain it so it makes sense? I was writing a natural
sorting routine so I could get some string array sorted. This is
because D is missing the capabilities to do a natural sort
correctly. It does not compare the the integer in string a with
the matching integer in string B correctly. If they happen to be
a different length then it assumes the shorter one was smaller.
At least, when I tried it, that is what I remember being the
problem, or some other weird behavior, but it was not correct.
Therefore, I decided to write the natural sorting routine to fix
that(This is because I am a person that wants to drink a full
glass of water rather than an empty glass).
To implement the algorithm is rather simple in some ways and
complicated in others.
string[] naturalSort(string[] arr) /*pure @safe*/
{
alias myComp = (x, y) {
return ComplexPart(x,y);
};
auto x = arr.sort!(myComp).release;
return x;
}
bool ComplexPart(string x, string y)
{
auto ml = cast(int)(min(x.length, y.length)-1);
auto ofsX = -1; auto ofsY = -1;
auto numX_found = -1;
auto numY_found = -1;
for(int i = 0; i <= ml; i++)
{
// If one string is left slice of the other, it wins
if (i >= ml || ofsX >= ml || ofsY >= ml) { if (x.length <
y.length) return true; return false; }
ofsX++;
ofsY++;
// If characters are not a digit, use default comparer
if (!isDigit(x[ofsX]) || !isDigit(y[ofsY]))
{
if (x[ofsX] != y[ofsY])
return x[ofsX] < y[ofsY];
continue;
}
// We now know that x and y are identical up to the digit
sequence. Extract these sequences then compare, if identical *in
value*(000 = 0, etc) we continue the compare, else we compare
the numbers and use that to specify the comparision quality
while (ofsX < ml && isDigit(x[ofsX])) { ofsX++; };
while (ofsY < ml && isDigit(y[ofsY])) { ofsY++; };
auto numX = x[i..ofsX];
auto numY = y[i..ofsY];
auto res = compareStringNum(numX, numY);
if (res != 0)
return (res != 1);
Re: All asserts need to have messages attached! Explicit as possible!
On Saturday, 8 July 2017 at 01:31:54 UTC, Seb wrote: On Saturday, 8 July 2017 at 01:01:38 UTC, Vladimir Panteleev wrote: On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote: It would be easy to find all the bad asserts? Does Dscanner have a rule to detect assert and enforce calls without a message? We should have it enabled for Phobos. No, but now there's: https://github.com/dlang-community/D-Scanner/pull/495 I can always add this to the phobos branch at DScanner if you want to test this immediately. We can improve the status quo: https://github.com/dlang/phobos/pull/5578 Once this is integrated, everyone can help out and improve Phobos by just removing a single module from the blacklist - it's a PR away.
Re: All asserts need to have messages attached! Explicit as possible!
On 2017-07-08 04:13, Jonathan M Davis via Digitalmars-d wrote: I tend to agree - in many cases, the error message is completely redundant - but a number of folks seem to think that you should never have an assertion without a message, and it tends to get complained about if you have an assertion without a message if it's not in a unit test, so I'm a bit surprised that the OP ran into one in Phobos. I think it's extremely useful to have, especially in a code base you're not familiar with or is complicated. It has happened to me quite often that I hit asserts in DMD when I develop on that code base. It would help me a lot if all those asserts had descriptive messages. -- /Jacob Carlborg
Re: All asserts need to have messages attached! Explicit as possible!
On Monday, July 10, 2017 1:28:44 PM MDT Jacob Carlborg via Digitalmars-d wrote: > On 2017-07-08 04:13, Jonathan M Davis via Digitalmars-d wrote: > > I tend to agree - in many cases, the error message is completely > > redundant - but a number of folks seem to think that you should never > > have an assertion without a message, and it tends to get complained > > about if you have an assertion without a message if it's not in a unit > > test, so I'm a bit surprised that the OP ran into one in Phobos. > > I think it's extremely useful to have, especially in a code base you're > not familiar with or is complicated. It has happened to me quite often > that I hit asserts in DMD when I develop on that code base. It would > help me a lot if all those asserts had descriptive messages. That only helps if there's something to describe. Often, it's something like assert(!range.empty); in which case, there really isn't much to say. I totally agree that assertion messages can be helpful when there's something to explain, but there often really isn't, and all a message is going to do is repeat the condition being checked in English, which is just pointless text. - Jonathan M Davis
Re: All asserts need to have messages attached! Explicit as possible!
On 2017-07-10 15:45, Jonathan M Davis via Digitalmars-d wrote: That only helps if there's something to describe. Often, it's something like assert(!range.empty); in which case, there really isn't much to say. I totally agree that assertion messages can be helpful when there's something to explain, but there often really isn't, and all a message is going to do is repeat the condition being checked in English, which is just pointless text. Well, in this case it could include why the range cannot be empty. But in the case of ranges it's pretty clear, if you know how the whole range concept works. -- /Jacob Carlborg
Re: All asserts need to have messages attached! Explicit as possible!
On Monday, 10 July 2017 at 13:52:22 UTC, Jacob Carlborg wrote: On 2017-07-10 15:45, Jonathan M Davis via Digitalmars-d wrote: [...] Well, in this case it could include why the range cannot be empty. If either the name of the function was non-descriptive or the function's documentation string (and assert messages are also just documentation) doesn't make it obvious why, sure. But in the case of ranges it's pretty clear, if you know how the whole range concept works. Even if you don't know about ranges, the function's name (pop*, remove*, extract*, etc.) virtually always implies a state mutating operation removing something.
