On Wed, 2018-03-28 at 22:50 +0200, Christophe de Dinechin wrote: > > On 28 Mar 2018, at 22:32, Christophe de Dinechin <cdupontd@redhat.c > > om> wrote: > > > > > > > > > On 28 Mar 2018, at 20:46, Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > > wrote: > > > > > > On Wed, 2018-03-28 at 17:42 +0200, Christophe de Dinechin wrote: > > > > Somewhat OT, but hopefully informative in the context of this > > > > thread. > > > > > > > > > > > > > On 26 Mar 2018, at 18:24, Jonathon Jongsma <jjongsma@redhat.c > > > > > om> > > > > > wrote: > > > > > > > > > > As teuf mentioned in a different email, std::runtime_error in > > > > > C++11 > > > > > has a const char* constructor: > > > > > > > > Since the presence of this constructor was used repeatedly as > > > > an > > > > argument, I’ve asked a few times if someone could tell me why > > > > this > > > > constructor was added in C++ 11. So far, nobody answered. I did > > > > not > > > > think it was so hard to google it. Oh well, since nobody else > > > > took up > > > > the challenge, time for me to answer the question… > > > > > > > > Here is one source giving a relatively short explanation: https > > > > ://sta > > > > ckoverflow.com/questions/29052647/c11-introduced-exception- > > > > constructors-taking-const-char-but-why. I quote: "This allows > > > > (or at > > > > least is apparently intended to facilitate--see more below) the > > > > implementation to eliminate copies in cases where it can detect > > > > (by > > > > means that are not themselves standardized) that what's being > > > > passed > > > > is a string literal or something else with static storage > > > > duration.” > > > > See also the note at the end of that answer: "[ Oxford: The > > > > proposed > > > > resolution simply addresses the issue of constructing the > > > > exception > > > > objects with const char* and string literals without the need > > > > to > > > > explicit include or construct a std::string. ]” > > > > > > > > So what happened is that the committee knew that having an > > > > std::string constructed for all runtime errors was problematic, > > > > as > > > > far back as 2000. This is defect 254, see http://std.dkuug.dk/j > > > > tc1/sc > > > > 22/wg21/docs/lwg-closed.html. It was a bit annoying if > > > > implementors > > > > took a great deal of care and effort to ensure that you could > > > > throw > > > > an exception reliably in low-memory situations (see section > > > > 4.2.2 of > > > > https://github.com/itanium-cxx-abi/cxx-abi/blob/master/HP-excep > > > > tion-9 > > > > 90818.pdf if you are curious about the kind of shenanigans I’m > > > > talking about) only to see all these effort thwarted by the > > > > (legacy) > > > > interface of std::runtime_error. > > > > > > > > However, you can imagine that back in 2000, deciding to remove > > > > the > > > > std::string constructor was difficult. So the problem was not > > > > easy to > > > > fix, and it shouldn’t be too hard to believe that there was a > > > > lot of > > > > discussion (254 is listed as one of the hot topics for the > > > > Redmond > > > > meeting). Before the Oxford meeting, the decision was to accept > > > > throwing bad_alloc as a fact of life. But as you can see at the > > > > bottom of bug 254, the consensus on a better longer-term > > > > resolution > > > > for C++0x, which became C++11, was to add a const char * that > > > > would > > > > allow the implementation to special-case std::runtime_error and > > > > avoid > > > > the copy. In the Oxford meeting, that resolution was confirmed, > > > > and > > > > the change made it to C++11. > > > > > > > > Out of curiosity, I checked both clang and g++, and they don’t > > > > actually store a string in std::runtime_error, I suspect in > > > > large > > > > part because of another related issue, namely the risk of > > > > throwing > > > > from string copy constructors. though they don’t seem to be > > > > doing the > > > > optimization either, because it’s quite hard relative to the > > > > benefits > > > > :-( So no worries, on clang, if you are really out of memory, > > > > you > > > > will not throw bad_allloc or end up in terminate(). No sir, the > > > > standard forbids it. No, instead, you will segfault writing > > > > into a > > > > null pointer (line 74 of https://llvm.org/viewvc/llvm-project/l > > > > ibcxx/ > > > > trunk/src/include/refstring.h?view=markup if you are curious). > > > > So > > > > much better. > > > > > > > > Bottom line: the very reason there is a const char * > > > > constructor in > > > > std::runtime_error is to provide a workaround for a known > > > > design bug. > > > > There is no value in blindly repeating that design bug. > > > > > > > > > > > > Cheers, > > > > Christophe > > > > > > > > _______________________________________________ > > > > Spice-devel mailing list > > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > > > > > > This doesn't seem like a fruitful discussion any longer. > > > > It’s long been unproductive, only blocking my patch series for no > > good reason. > > > > > I disagree > > > with your opinion that *for our use cases* it's more desirable to > > > avoid > > > a small memory allocation than to make the class more resilient > > > to > > > programmer error. But you clearly won't be convinced, and it's > > > not the > > > most important issue in the world. So I'll withdraw from this > > > discussion. > > > > And I find it distasteful that you still insist using std::string > > would make it “more resilient to programmer error” when I’ve proven > > it was not the case and spent an inordinate amount of time > > explaining why. > > By which I mean among other things what I wrote earlier today: > > > Therefore, it is important for the interface of Error to “strongly > > suggest” constant strings where I want constant strings. By the > > mean of a “const char *”. Where I expect a dynamic string, I use > > std::string (or const std::string & or whatever floats you boat). > > As long as you keep ignoring that point, as long as you keep ignoring > that detecting an inadvertent use of std::string input is actually > *by design* and *desirable*, you have not understood that your > proposal *in our use case* makes things significantly *less* > resilient to the most important class of programmer error, where a > programmer would mistakenly attempt to pass something like “can’t > write “ + file. > > So you will keep blocking this patch indefinitely because you don’t > understand it. And I find this unacceptable. > OK, let's try to be more productive here. Please accept this email as a good-faith attempt to reach concensus. To recap briefly, my suggestion was simply to store the string passed to the Error constructor internally so that we did not segfault if that string was freed by the caller before the exception was caught. If I understand your position, the argument AGAINST this is basically: 1. It requires additional memory allocation (which introduces the potential of throwing a std::bad_alloc exception while attempting to throw a different exception) 2. The code is slower and more bloated (due to additional memory allocation, or e.g. constructing a new std::string for internal storage) 3. It "encourages" people to use dynamically allocated strings for error messages which is inherently bad. Is that accurate? Am I missing any? I think #3 is your primary argument, right? For now I'm going to assume that I represented your views accurately and I'll briefly address each of these points below. Now, the argument FOR copying the string is basically: A. we don't want a segfault if somebody passes a string that's not a static constant. I have a really hard time caring about the bad_alloc issue. If we fail to allocate enough memory for a simple error message, we are going to have much bigger problems. So I don't consider #1 to be a significant issue. I also don't really really care about performance in exception- handling situations, so #2 is also a non-issue for me. That basically leaves me to decide whether it's more important discourage using dynamic strings or whether it's more important to avoid segfaults when somebody does use dynamic strings. But as I thought about it more, I found that I'm a bit confused here about exactly why you consider dynamic strings to be evil. Is it just because they can't be translated? Certainly, to be translatable, the *template* for an error message must be a static constant. But if we want to include any useful context to the user, the error message itself must be somewhat dynamic. In other words, the error message template "Failed to read file %s" is a static constant, but the error message itself ("Failed to read file /home/cddd/foo.txt", or the equivalent in russian, etc) is necessarily a dynamic string. (If this is not the reason that you insist on static strings, please enlighten me, because clearly I'm missing something.) In any case, your patch also uses "dynamic" strings as error messages, but it does so in a slightly different way. You store a pointer to the static error message *components* within the Error class, but then you format the message later (when the exception is caught) to create the dynamic error message. According to your earlier responses on this topic, you claim that this is done to avoid memory allocation during the exception handling process (since format_message() uses a buffer provided by the exception handler rather than allocating its own buffer) and to improve performance by delaying formatting until we actually need to do it. But these issues essentially map to #1 and #2 above, which I've already said I don't really care about. In addition, it introduces a discrepency between the message that is returned by what() and the message that is returned by format_message(), which I find undesirable. So in my opinion, the lazy format_message() feature adds complexity for very little gain. In my opinion, we should simply translate and format the full error message at the point of throw and pass this formatted and translated string to the Error constructor. throw WriteError(format(_("Write failed writing header for file '%s'"), filename)); I think this has a number of advantages. * It's simpler * Since the message is already translated, we don't need to worry about whether the string is static or dynamic (right?) * Following on from the last point, since we don't have to discourage dyanamic strings anymore, we can copy the string and store it internally to avoid potential segfaults * The what() function would return the fully formatted error message rather than just the static portion of the message. * It's easier to translate the strings [*] The only drawbacks in my mind are the memory allocation and performance issues I mentioned above. And I personally think that the advantages I mentioned above far outweigh those disadvantages. Jonathon [*] In an earlier email, you gave an example of how you might handle translations with your lazy formatting approach, but I'm not actually sure that it would work. Quoting your email: This naive approach yields “l’écriture a été interrompue pendant le format pour le fichier ‘Hello.txt'”, which is understandable, but sounds a bit strange. It would be better to instead generate “L’écriture du format a été interrompue pour le fichier ‘Hello.txt’”; which involves word reordering and grammatical pairing (e.g. contractions, male/female, etc). In order to do that, you can take a more subtle approach where instead of translating pieces individually, you translate the complete message, but still carefully ignore the parts that you know are variable. So you could do instead: snprintf(translated, sizeof(translated), “%s writing %s for file ‘%%s’”, what(), operation); int written = snprintf(buffer, size, _(translated), filename.c_str()); return append_strerror(buffer, size, written); So you envision building the translatable strings from the static components that you pass to the error constructor. But notice that the variable "translated" is not a static constant string, so I don't think that "_(translated)" would actually work. Maybe this example was not intended to be taken literally? But I can't think of a good way that you could extract a list of static constant strings from the code in a way that would work with gettext. _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel