| From: Andrew Cagney <andrew.cag...@gmail.com>
| On Sat, 19 Jan 2019 at 03:51, D. Hugh Redelmeier <h...@mimosa.com> wrote:
| >
| > | From: Andrew Cagney <andrew.cag...@gmail.com>
| > It doesn't make sense to initialize a static with an assignment from
| > an object. Amusingly, it does make sense to initialize a static with
| > the address of a compound literal!
| >
| > I think GCC makes sense of initializing a static with a compound literal
| > unless it has told to be strict in some way. We always want GCC
| > strictness so more of our errors will be caught.
|
| Surely it is better to write code compliant with the language spec,
| and not according to what a specific compiler seems to accept (unless
| there's a really compelling case and evidence that LLVM also accepts
| the same construct)?
I certainly agree. I was not advocating using an object as a static
initializer, only observing that it sometimes works with GCC. That's
how I compiled code using this feature and thus blithely assumed it
was proper C (since fixed).
| Here, compound literals aren't valid in static initializers but gcc
| _sometimes_ accepts them. We need a simple consistent way of
| achieving the same result.
Right.
| > | Through trial and error (read, being repeatedly pinged for the same
| > | mistake on IRC cf eb3ba97984910933b3ce9ff3155c2932b48597a0), I've
| > | found the following works best:
| > |
| > | - lower case function or variable - here chunk(), empty_chunk, ... -
| > | use this in all dynamic code
| > | - upper case macro - here CHUNK(), EMPTY_CHUNK, ... - use this in all
| > | static code
| >
| > I take it by "use this in all static code" you really mean "use this
| > as an initializer". In other words, it isn't an expression.
| >
| > That's not our past convention and I don't immediately see why to use
| > it.
|
| Er.. But our past convention was to:
| - ban dynamic initializers
I don't think that was the case. If I remember correctly, we didn't
use dynamic compound initializers because they were not part of the
original C Standard. The standard allowed autos to be initialized by
a run-time expression, but not if the initializer involved braces. If
the initializer involved braces, the values had to be statically
evaluable (constants or addresses of statics).
| - require static initializers to be fully populated and not use named fields
That was the case 15 years ago, but it had changed before you joined
the project.
Confusingly, some code used the GCC extension for named fields and not
the C standard's way. I don't remember when that got fixed.
| So prior to me using these new language features, and better
| abstracting primitives like realtime_t, this hadn't even come up. And
| in doing this, I started following a convention you clearly don't
| like.
I don't like macros expanding to things that are not expressions or
statements. It is sometimes worth doing but I wish to minimize it and
to make it quite clear when it is done.
Footnote:
The biggest change that you catalyzed was to allow definitions to be
intermingled with other statements within a block. I like that a lot.
| > REALTIME_EPOCH is not a constant: it cannot be used in a normal
| > expression. It is an initializer. And it is used only once: hardly
| > worth defining. But writing it in upper case does conform: it doesn't
| > work like a function.
|
| Off the top of my head there's REALTIME_EPOCH, MONOTIME_EPOCH, and
EMPTY_CHUNK.
Thanks for that list. I discussed REALTIME_EPOCH and EMPTY_CHUNK
previously.
MONOTIME_EPOCH is used three times, two of which are static
initializers. The other one could be replaced by "monotime_epoch" (or
by MONOTIME_EPOCH if that expanded to a compound literal).
One of those static initializers is outside of monotime.c. So
MONOTIME_EPOCH is protecting an abstraction in that case. So there is
a good argument for keeping it.
I strongly recommend that MONOTIME_EPOCH be renamed to indicate that
it is syntactically an initializer and not an expression.
MONOTIME_EPOCH_INITIALIZER would be reasonable but other naming
conventions might be better. I'm assuming that this is the start of a
convention.
| So take a step back and look at them as a group.
| However, I can be confident that:
|
| const chunk_t empty_chunk = EMPTY_CHUNK;
|
| yields the same value as EMPTY_CHUNK used elsewhere.
Lets be careful here.
EMPTY_CHUNK (when it was an initializer) didn't yield a value -- it was
not an expression.
"const chunk_t empty_chunk = EMPTY_CHUNK;" doesn't yield a value, it's
a declaration, not an expression. Now that EMPTY_CHUNK expands to a
compound literal, it isn't even legal.
empty_chunk was not equal to EMPTY_CHUNK (when it was an initializer)
since EMPTY_CHUNK wasn't an expression.
empty_chunk is equal to EMPTY_CHUNK now that EMPTY_CHUNK expands to a
compound literal.
So: I don't know what you mean by "yields the same value as EMPTY_CHUNK
used elsewhere".
Footnote:
If EMPTY_CHUNK_INITIALIZER were defined in the obvious way,
EMPTY_CHUNK and empty_chunk could be defined using it. If we ditch
EMPTY_CHUNK or empty_chunk (we don't really need both), there is no
need for a separate EMPTY_CHUNK_INITIALIZER: it could be inlined into
the definition of empty_chunk or EMPTY_CHUNK, both of which are
inside the abstraction.
| And as I've had to point out in the past, just because something isn't
| used, or is only used once is not good reason to delete it.
If the thing isn't used and won't be used, it should be deleted.
If it isn't used but is expected to be used in the future, comment it
out. (Two advantages: the reader and compiler can ignore it; the
first user knows that it has not been tested.)
If the thing is used once, and it doesn't protect an abstraction or
simplify the reading of the code, it should be inlined.
| > | When I tried to cast something like EMPTY_CHUNK so that it, instead of
| > | empty_chunk, could be used (a foolish premature optimisation on my
| > | part), I was invariably forced to either:
| > |
| > | - define a further macro with no cast - above avoids needing to do this
| > | - or hard-wire those cases - as has happened here - defeating the
| > | point of the abstraction
| >
| > I don't understand this point.
| >
| > EMPTY_CHUNK is a compound literal. It works everywhere empty_chunk
| > would work. Neither would work as a static initializer.
| >
| > There is one exception. The address of empty_chunk would always be
| > the same whereas the address of EMPTY_CHUNK would always be different.
| > But I don't think any of our code would care.
| >
| > | I don't think performance and/or static analysers are an issue:
| > |
| > | - given their price, I'd be really surprised if a static analyser,
| > | which looks at the entire code base, couldn't figure out that:
| > | return empty_chunk;
| > | and
| > | return (chunk_t) { NULL,0, }
| > | are the same
| >
| > As far as I know we don't run the compiler in "whole program" mode.
|
| GCC isn't a static analyser.
It sure is. Static analysis is one thing every compiler does to a greater
or lesser extent. GCC is the static analyzer that we use the most and it
does a fairly good job.
| You offer no evidence that GCC can make use of this.
No. I don't care because I try not to code to match a compiler (a
moving target), I code to match the language. I also have some idea of
what compilers could do.
| As you point out
| it isn't doing whole program static analysis so can't see 'NULL' being
| returned from a function in another file.
I agree but don't understand the relevance of this observation.
| You offer no evidence to support this making pluto materially faster.
| OTOH, I can point to specific performance problems and they all have
| zero to do with these sorts of changes.
I made two claims: it makes it possible for a compiler to produce
better code (faster and smaller) and it makes it possible for static
analysis to produce better diagnostics. I strongly resist coding for
a specific compiler.
I also think that it makes it easier for a human reader to understand.
| > | OTOH, I can print empty_chunk (but not EMPTY_CHUNK)
in GDB (in Pluto you could print empty_chunk and EMPTY_CHUNK with
identical methods)
| > | and can reliably
| > | breakpoint and step over a chunk() function (but not the alternatives,
| > | at least not reliably)
| >
| > Why would you want to? These are tiny little transparent operators.
| > You know from inspection just what they do.
|
| Because it makes debugging easier.
I don't use gdb much. Half the times when I do, it just doesn't work
for me (autos optimized out).
I know that you are an adept: you even wrote some of it.
Why would you ever have to ask GDB what EMPTY_CHUNK yields? It only has
one rvalue, for ever-and-ever.
Your reason suggest that we'd be better off replacing + with a
function call. The results of + are certainly more informative than
the result of EMPTY_CHUNK.
| I find it troubling that I need to
| answer this.
How so?
I'm sorry if I've offended you.
_______________________________________________
Swan-dev mailing list
Swan-dev@lists.libreswan.org
https://lists.libreswan.org/mailman/listinfo/swan-dev
