Re: Proposed addition to CodingStyle web page about assert(a && b)
On Sun, May 5, 2019 at 7:18 AM walter harms wrote:
> Am 05.05.2019 09:11, schrieb Matthieu Herrb:
> > On Sat, May 04, 2019 at 03:47:31PM -0700, Adam Richter wrote:
> >> Hi, everyone.
> >>
> >> I would like to propose that whoever has the ability to edit the web
> >> page add a line like the following to
> >> https://www.x.org/wiki/CodingStyle/ :
> >>
> >> - Separate assert(a && b) into assert(a) and assert(b).
> >>
> >>
> >> Thanks in advance for any input on this.
> >
> > Hi,
> >
> > I'm not sure if this advice belongs to this wiki page which is more
> > oriented on the appearance of the code than on semantics or
> > development good practices.
> >
> > On the development good practices side, I think assert() should be
> > banned as much as possible form libraries and drivers.
> >
> > You don't know anything about the caller context and having it beeing
> > brutally abort()ing is brutal and my lead to security issues
> > (data leaks in the core file for instance) or data corruption.
> >
> > In libraries assert() should never be used to reject bad user input or
> > any other error condition that can happen for some known reason. It
> > should really only be used to document conditions that should really
> > never happen. In all other cases the function should be able to return
> > an error to the caller (which should of course not ignore them).
> >
> >
>
> i do not comment on the use of assert() generally, it can be used
> by anyone who likes that. Things are getting problematic when use
> like this:
>
>assert(0 < asprintf(, "%s/Library/Logs/X11", home));
>
> this is simply dangerous as you can define NDEBUG and let everything vanish.
>
> BTW are the libraries routinely compiled with NDEBUG enabled ?
>
> re,
> wh
>
>
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
Matthieu, thank you for pointing out that most of that CodingStyle web
page is about page layout. The last entry on that page is about
program practices, but that seems to be an exceptional situation
calling for a reversal of a previous X development convention. I did
not notice it before your comment, but I see now that that page really
is not a collection of practical semantics recommendations. So,
unless anyone would like to advocate otherwise, I drop my request for
an edit of that CodingStyle page.
Matthieu and Walter, thank you for your input on the point the use of
assert. I think I mostly agree with both of you. Where I might
disagree ("might" because I don't know your views on this, so maybe we
already agree) is that I think that assert() is almost always much
better than doing nothing, so I would encourage people to write assert
statements first, if that's easier, and then revisit them to see which
assert statements can be converted to more controlled error handling.
By offering this opinion, I don't mean to imply that my opinion needs
to carry any weight; I'm just offering thoughts for you to consider.
With that said, I'm not quite sure what to do with my darwin.c changes
to replace some assert statements, including the one that Walter used
as an example, with code that calls FatalError() if the operation in
question fails, given that I have not easily found an OSX system on
which to compile it. So, I'll ask now, would anyone object to my
submitting that darwin.c patch (darwin-assert-v2.diff earlier in this
thread) under this circumstance? If nobody complains or requests I do
otherwise in the next ~24 hours, I guess I'll go ahead and submit that
patch on gitlab.freedesktop.org.
Adam
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Proposed addition to CodingStyle web page about assert(a && b)
Am 05.05.2019 09:11, schrieb Matthieu Herrb: > On Sat, May 04, 2019 at 03:47:31PM -0700, Adam Richter wrote: >> Hi, everyone. >> >> I would like to propose that whoever has the ability to edit the web >> page add a line like the following to >> https://www.x.org/wiki/CodingStyle/ : >> >> - Separate assert(a && b) into assert(a) and assert(b). >> >> >> Thanks in advance for any input on this. > > Hi, > > I'm not sure if this advice belongs to this wiki page which is more > oriented on the appearance of the code than on semantics or > development good practices. > > On the development good practices side, I think assert() should be > banned as much as possible form libraries and drivers. > > You don't know anything about the caller context and having it beeing > brutally abort()ing is brutal and my lead to security issues > (data leaks in the core file for instance) or data corruption. > > In libraries assert() should never be used to reject bad user input or > any other error condition that can happen for some known reason. It > should really only be used to document conditions that should really > never happen. In all other cases the function should be able to return > an error to the caller (which should of course not ignore them). > > i do not comment on the use of assert() generally, it can be used by anyone who likes that. Things are getting problematic when use like this: assert(0 < asprintf(, "%s/Library/Logs/X11", home)); this is simply dangerous as you can define NDEBUG and let everything vanish. BTW are the libraries routinely compiled with NDEBUG enabled ? re, wh ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Proposed addition to CodingStyle web page about assert(a && b)
On Sat, May 04, 2019 at 03:47:31PM -0700, Adam Richter wrote: > Hi, everyone. > > I would like to propose that whoever has the ability to edit the web > page add a line like the following to > https://www.x.org/wiki/CodingStyle/ : > > - Separate assert(a && b) into assert(a) and assert(b). > > > Thanks in advance for any input on this. Hi, I'm not sure if this advice belongs to this wiki page which is more oriented on the appearance of the code than on semantics or development good practices. On the development good practices side, I think assert() should be banned as much as possible form libraries and drivers. You don't know anything about the caller context and having it beeing brutally abort()ing is brutal and my lead to security issues (data leaks in the core file for instance) or data corruption. In libraries assert() should never be used to reject bad user input or any other error condition that can happen for some known reason. It should really only be used to document conditions that should really never happen. In all other cases the function should be able to return an error to the caller (which should of course not ignore them). -- Matthieu Herrb signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Proposed addition to CodingStyle web page about assert(a && b)
Hi, everyone. I would like to propose that whoever has the ability to edit the web page add a line like the following to https://www.x.org/wiki/CodingStyle/ : - Separate assert(a && b) into assert(a) and assert(b). I can think of several potential advantages of separating logical conjunctions in assertions: 1. Assertion failures are often sporadic, and users who report them may not be in a position to efficiently narrow them down further, so it is important to get as much precision from each assertion failure as possible. 2. It is a more efficient use of developers time when a bug report arrives if the problem has been narrowed down that much more. A new bug report may initially attract more interest, so, if the problem has been narrowed down that much more, it may increase the chance that developers may invest the time to try to resolve the problem, and also reduce unnecessary traffic on the developer mailing list about possible causes of the bug that separating the assertion was able to rule out. 3. It's often more readable, sometimes eliminating parentheses or changing multi-line conditions to separate single line conditions. 4. When using a debugger to step over an assertion failure in the first part of the statement, the second part is still tested. 5. Providing separate likelihood hints to the compiler in the form of separate assert statements does not require the compiler to be quite as smart to recognize that it should optimize both branches, although I do not know if that makes a difference for any compiler commonly used to compile X (that is, I suspect that they are all smart enough to realize is that "a && b" is likely true, then "a" is likely true and "b" is likely true). With that said, I have noticed at least one clever reasonable exception to this, which is an assert statement like this one in hw/xfree86/drivers/modesetting/drmmode_display.c, where the second condition is just a string that that the develop wants to have included in that failure message: assert(num_infos <= 32 && "update return type"); I would welcome some way to state that in the coding style that is nearly as concise as the way the other lines in that web page are written. As a bit of background, I want to mention that I went on a mostly successful similar campaign for BUG_ON(a || b) in the Linux kernel over the course of a week or so about a decade ago but never bothered to ask for a change in the coding guidelines there, and a few days ago I noticed that some statements of that form have crept back in. So, learning from that experience, I figure I should at least ask to add this to the coding style document. To avoid duplication of effort, I will also mention that I have already submitted a change on gitlab to separate such assertions in the core X server and am about to submit a similar patch for xf86-video-intel. Thanks in advance for any input on this. Adam ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
