[squid-dev] [PATCH] implement RFC3986

[Alex Rousskov]

On 03/16/2016 11:33 AM, Kinkie wrote:
>>> Will it be initialized at all? I'd expect that fromHexTable, which is
>>> const and POD be simply laid out in the data segment and not require
>>> initialization at all.
>>
>> Are you implying that
>>
>> (a) fromHexTable is a C++11 constexpr _and_
>> (b) constexpr cannot suffer from initialization order problems?
>>
>> If yes, then you should declare fromHexTable as constexpr so that
>> somebody does not accidentally change it to be something else. This will
>> make (a) a strong explicit statement rather than an implied and fragile
>> implication.

> both are list of constants. No calls to any code of any kind.

Yes, today, they are, but there is no indication that it is an important
property or invariant. Somebody can replace those initialization with a
function call tomorrow and nobody may notice that mistake until bug
reports start coming in.


> toHexTable can't be declared constexpr: g++ complains that the standard
> doesn't allow that for strings.

Which, to me, is an indication that we should not _assume_ anything
about its initialization timing! We have to be careful here.

BTW, "constexpr const char *toHexTable" appears to compiler fine for me,
but perhaps my g++ is too old or this is the wrong way to add constexpr
to toHexTable? There is a somewhat related discussion to this at
http://stackoverflow.com/questions/30561104/const-constexpr-char-vs-constexpr-char


>> However, I cannot find any C++ rule that guarantees the behavior you
>> expect -- your fromHexTable (even if you add constexpr to it) does not
>> seem to match any of the three items at:
>>
>>   http://en.cppreference.com/w/cpp/language/constant_initialization


> To me they seem both cases of (quote)
> 3) Static or thread-local object (not necessarily of class type),

That rule did not seem to match for me because the tables in the v2
patch I was reviewing where not marked as "static". You have changed
their declarations now so the above does seem to apply. Thank you.

Please note that I am _not_ saying that rule #3 did not apply before
your change. I am saying that it did not seem to apply. Writing correct
code is only a part of what we have to do; we have to make our code
appear to be correct to others.


> This stems from my understading of the meaning of the ".data" section
> of ELF files, which may be partial or incomplete.

... and, more importantly, has nothing to do with C++. The .data section
may store all our constants, but that does not mean those constants
cannot be copied to some of our objects "dynamically", at some
semi-random time after the program starts.


> Unfortunately initialization rules are quite hard for me to understand yet.

They are hard for everybody! I am surprised you volunteered to fix an
initialization problem, but since you have done so, it is not fair to
avoid the rules that are required to fix it (including proving that your
fix is correct).


>>> +const int16_t fromHexTable[256] = {
>>
>> AFAICT, this needs to be "static" and should be "constexpr" to (a)
>> guarantee constant initialization and (b) minimize the chances of
>> somebody changing it to something that will not be initialized at
>> "constant initialization" time. Please correct me if I am wrong.

> Aren't variables not declared extern marked static by default?

Good question [that we should not be asking IMHO].

AFAICT[1], by default, variables have external linkage (although C++
complicates that further) and static storage duration. Does the "Static
or thread-local object" phrase in the C++ constant initialization rule
#3 at [2] talk about storage duration or linkage? I think it is about
storage duration.

[1]
http://stackoverflow.com/questions/3281925/what-is-default-storage-class-for-global-variables

[2] http://en.cppreference.com/w/cpp/language/constant_initialization


> Sure can do but it should be redundant.

It may be redundant for the compiler, but if you think "static" is
redundant for us, then either you have no respect for the hours wasted
discussing three versions of your patches or you think I was missing
something that would be obvious to everybody else.

I still do not understand why the tables should not go inside the
functions that use them, eliminating all questions. However, I think
patch v3 does not suffer from table initialization order problems.


What about CharacterSet globals like Rfc1738::Unsafe? Do you have
reasons to believe they will be initialized before any possible first
use? For example, if some Foo.cc contains the following global, will it
always be initialized correctly?

  #include "anyp/Rfc3986.h"
  const CharacterSet FooSet = Rfc3986::Reserved + Rfc1738::Ctrls;

I do not think so because none of the three constant initialization
rules[2] appear to apply to Rfc3986::Reserved (for example).


Thank you,

Alex.