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.