Re: [Spice-devel] [spice-gtk v1 09/11] channel-display: don't debug latency for each frame

[Victor Toso]

On Tue, Mar 13, 2018 at 12:25:40PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Becomes quite hard to find meaning on something that is printed every
> time. Only print latency value if it is a new min/max or if average
> latency is 10% bigger/lower then usual.
> 
> Not aiming to perfect statistics in latency here, just to avoid too
> verbose logging. Removing latency debug isn't cool as we could infer
> issues with streaming based on it.
> 
> Signed-off-by: Victor Toso 
> ---
>  src/channel-display-priv.h |  3 +++
>  src/channel-display.c  | 18 +-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 95ad7d8..e7758cc 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -136,6 +136,9 @@ struct display_stream {
>  drops_sequence_stats cur_drops_seq_stats;
>  GArray   *drops_seqs_stats_arr;
>  uint32_t num_drops_seqs;
> +uint32_t latency_min;
> +uint32_t latency_max;
> +uint32_t latency_avg;

Btw, the latency_avg should be double.

>  
>  uint32_t playback_sync_drops_seq_len;
>  
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 4757aa6..3901cd1 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1547,6 +1547,10 @@ static void display_stream_stats_save(display_stream 
> *st,
>guint32 client_mmtime)
>  {
>  gint32 latency = server_mmtime - client_mmtime;
> +gint32 min_latency = st->latency_min == 0 ? latency : 
> MIN(st->latency_min, latency);
> +gint32 max_latency = MAX(st->latency_max, latency);
> +gdouble avg_latency = (st->latency_avg * st->num_input_frames + latency) 
> /
> +  ((double) (st->num_input_frames + 1));
>  
>  if (!st->num_input_frames) {
>  st->first_frame_mm_time = server_mmtime;
> @@ -1567,7 +1571,19 @@ static void display_stream_stats_save(display_stream 
> *st,
>  return;
>  }
>  
> -CHANNEL_DEBUG(st->channel, "video latency: %d", latency);
> +/* Only debug latency value if it matters otherwise it can be too 
> verbose */
> +if (min_latency != st->latency_min ||
> +max_latency != st->latency_max ||
> +avg_latency < 0.90 * st->latency_avg ||
> +avg_latency > 1.10 * st->latency_avg) {
> +CHANNEL_DEBUG(st->channel,
> +  "video latency: %d | (%d , %0.2f , %d)",
> +  latency, min_latency, avg_latency, max_latency);
> +st->latency_min = min_latency;
> +st->latency_max = max_latency;
> +}
> +st->latency_avg = avg_latency;
> +
>  if (st->cur_drops_seq_stats.len) {
>  st->cur_drops_seq_stats.duration = server_mmtime -
> 
> st->cur_drops_seq_stats.start_mm_time;
> -- 
> 2.16.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 00/11] misc improvements in channel-display

[Victor Toso]

Hi,

On Tue, Mar 13, 2018 at 12:25:31PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Hi,
> 
> The first four patches are related to using hash table to track stream's
> structure, discussed with Lukas at [0]. The others are trying to:
> - reduce too much code in one place;
> - reduce the verbose logs we have with streaming;
> 
> [0] https://lists.freedesktop.org/archives/spice-devel/2018-March/042529.html

Friendly ping.

> Thanks for your input,
> toso
> 
> Victor Toso (11):
>   channel-display: remove id parameter from helper function
>   channel-display: rename display_stream_destroy()
>   channel-display: remove unneeded function
>   channel-display: use GHashTable to keep stream's structure
>   channel-display: add spice_frame_free() helper
>   channel-display: add spice_frame_new() helper
>   channel-display: add display_stream_stats_save() helper
>   channel-display: add display_stream_stats_debug() helper
>   channel-display: don't debug latency for each frame
>   channel-display-gst: summarize number of frames dropped
>   channel-display-mjpeg: remove verbose logs
> 
>  src/channel-display-gst.c   |  19 +--
>  src/channel-display-mjpeg.c |  24 +---
>  src/channel-display-priv.h  |   6 +-
>  src/channel-display.c   | 313 
> ++--
>  4 files changed, 201 insertions(+), 161 deletions(-)


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice v2 1/1] rename the virtio port for streaming

[Victor Toso]

Hi,

On Fri, Mar 23, 2018 at 04:40:28PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 23 Mar 2018, at 15:57, Lukáš Hrázký  wrote:
> > 
> > From: Lukáš Hrázký 
> > 
> > The name 'com.redhat.stream.0' is too generic and in no way denotes it
> > belongs to SPICE. It is preferred to have the project's domain in the
> > name and Red Hat doesn't own the project. Rename it to
> > org.spice-space.stream.0.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> > server/reds.c | 2 +-
> > server/tests/test-stream-device.c | 2 +-
> > spice-common  | 2 +-
> > 3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 998f2ffa..935448d8 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3145,7 +3145,7 @@ static int 
> > spice_server_char_device_add_interface(SpiceServer *reds,
> > else if (strcmp(char_device->subtype, SUBTYPE_PORT) == 0) {
> > if (strcmp(char_device->portname, "org.spice-space.webdav.0") == 0) 
> > {
> > dev_state = spicevmc_device_connect(reds, char_device, 
> > SPICE_CHANNEL_WEBDAV);
> > -} else if (strcmp(char_device->portname, "com.redhat.stream.0") == 
> > 0) {
> > +} else if (strcmp(char_device->portname, 
> > "org.spice-space.stream.0") == 0) {
> 
> In addition to Frediano’s remarks, I would probably accept the
> two names during some transition period.

There were 0 releases in spice server with this. If this only
affect developers, I would prefer to keep it simple with only the
new name allowed.


> Otherwise, we are going to make everybody outside of our team
> miserable (and inside too, while we juggle configurations
> around this patch).

The warning in the log should be a good hint that this has
changed!

toso

> 
> > dev_state = RED_CHAR_DEVICE(stream_device_connect(reds, 
> > char_device));
> > } else {
> > dev_state = spicevmc_device_connect(reds, char_device, 
> > SPICE_CHANNEL_PORT);
> > diff --git a/server/tests/test-stream-device.c 
> > b/server/tests/test-stream-device.c
> > index 3c9209a4..2fdd0a39 100644
> > --- a/server/tests/test-stream-device.c
> > +++ b/server/tests/test-stream-device.c
> > @@ -107,7 +107,7 @@ static SpiceCharDeviceInterface vmc_interface = {
> > // this specifically creates a stream device
> > static SpiceCharDeviceInstance vmc_instance = {
> > .subtype = "port",
> > -.portname = "com.redhat.stream.0",
> > +.portname = "org.spice-space.stream.0",
> > };
> > 
> > static uint8_t *add_stream_hdr(uint8_t *p, StreamMsgType type, uint32_t 
> > size)
> > diff --git a/spice-common b/spice-common
> > index 4c2d0e97..45e28448 16
> > --- a/spice-common
> > +++ b/spice-common
> > @@ -1 +1 @@
> > -Subproject commit 4c2d0e977272c5540634d24f485dd64c424f6748
> > +Subproject commit 45e2844845242b32b2bd8956da0dfffa91c0d856
> > -- 
> > 2.16.2
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check

[Christophe Fergeau]

On Thu, Mar 29, 2018 at 10:36:06AM +0200, Christophe de Dinechin wrote:
> 
> 
> > On 29 Mar 2018, at 10:05, Christophe Fergeau  wrote:
> > 
> > On Wed, Mar 28, 2018 at 05:54:21PM +0200, Christophe de Dinechin wrote:
> >>> With that said, I find the current "ODR" portion of the commit log
> >>> misleading, it's easy to get the impression that after this change, we
> >>> won't have any ODR problem anymore (this is what I initially thought!).
> >> 
> >> Why do you find it misleading?
> >> 
> >> The precise wording is "The current plugin version check violates the
> >> C++ ODR”. It specifically talks about “current plugin version check”.
> >> And there is indeed no ODR violation in the version check after that
> >> change.
> >> 
> >> Whether there are ODR violations elsewhere, and whether we accept such
> >> violations purposefully on a case by case basis for other calls is a
> >> judgement call. The mechanism makes it entirely possible to avoid ODR
> >> violations completely (at the cost of unnecessarily restricting binary
> >> compatibility).
> >> 
> >> So I don’t see it as misleading or implying that it magically gets rid
> >> of other ODR violations.
> > 
> > As long as we don't change the ABI, the version check is as much
> > an ODR violation as any other part of the code.
> 
> In the present code, we violate the ODR *before* being able to assess if 
> that’s an ODR violation we are OK with.

Yes, this is also what I was saying ;) Though we could alternatively
mandate that the first few fields of the class must never change, in
which case the version check would still be an ODR violation, but one we
are fine with.

> In any case, could you suggest a wording that you think would be less 
> “misleading”?

I would use something like this:

 2. ODR-related problems

The C++ One Definition Rule (ODR) states that all translation units
must see the same definitions. In the current code, when we call
any Agent method from the plugin from the plugin, it is an ODR
violation as soon as we have made any change in the Agent class
compared to what the plugin was compiled against. As long as the
agent/plugin ABI does not change, this is deemed acceptable and
should not cause actual runtime issues.

However, the current version check (which is done through
Agent::PluginVersionIsCompatible) relies on the layout of the Agent
class not changing, which puts unnecessary constraints on the changes
allowed in the classes when breaking ABI (e.g. it's not allowed to
put anything before some member functions). Given that this version
check is used to detect ABI incompatibilities, it's better to make
it rely as little as possible on the ABI.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] gstreamer for view/record spice session

[Victor Toso]

On Wed, Mar 28, 2018 at 08:25:59PM +0300, Krutskikh Ivan wrote:
> Hi!
> 
> Is it possible ro use gstreamer to view or record spice screen session?

Not yet, but RFE is open at
https://bugs.freedesktop.org/show_bug.cgi?id=51714


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

[Christophe Fergeau]

On Wed, Mar 28, 2018 at 05:35:35PM +0200, Christophe de Dinechin wrote:
> > On 28 Mar 2018, at 17:04, Christophe Fergeau  wrote:
> > The part I'm missing is how you extract this limited number of full
> > strings that we'll need to translate into a po file (or equivalent). Is
> > there a tool which will generate that list of strings from %s writing
> > %s for file ... and the static strings which are passed as %s arguments to
> > snprintf?
> 
> Would the answer to this question have any chance of tilting the
> balance back towards changing the type of ‘message’ to std::string in
> the Error class? If not, why are you even asking?

You brought back multiple times the topic of localization, this made me
think this was an important part of the design which we might want to
discuss. I'm fine if we stop mentioning l10n in the discussion of this
patch series, I probably have already suggested that in the past :)

> 
> That being said, if you ask me if I believe that our code is presently
> in an “easily translatable” state, no, it is not. If you ask me if my
> patch is fixing that, it is not, and was never advertised as doing so.
> However, I’d argue that it moves the cursor from “impossible to
> translate” to either “tedious to translate” or “easy to translate with
> poor translations”.

Hmm I'll beg to disagree here, adding fairly standard gettext support to
git master seems quite easy, I experimented with it when localization
was mentioned a few weeks back:
https://gitlab.com/teuf/spice-streaming-agent/commits/i18n

As indicated by my earlier question, with your proposed Error design,
I'm not so sure how we could add good translation support.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check

[Christophe de Dinechin]

> On 29 Mar 2018, at 10:05, Christophe Fergeau  wrote:
> 
> On Wed, Mar 28, 2018 at 05:54:21PM +0200, Christophe de Dinechin wrote:
>>> With that said, I find the current "ODR" portion of the commit log
>>> misleading, it's easy to get the impression that after this change, we
>>> won't have any ODR problem anymore (this is what I initially thought!).
>> 
>> Why do you find it misleading?
>> 
>> The precise wording is "The current plugin version check violates the
>> C++ ODR”. It specifically talks about “current plugin version check”.
>> And there is indeed no ODR violation in the version check after that
>> change.
>> 
>> Whether there are ODR violations elsewhere, and whether we accept such
>> violations purposefully on a case by case basis for other calls is a
>> judgement call. The mechanism makes it entirely possible to avoid ODR
>> violations completely (at the cost of unnecessarily restricting binary
>> compatibility).
>> 
>> So I don’t see it as misleading or implying that it magically gets rid
>> of other ODR violations.
> 
> As long as we don't change the ABI, the version check is as much
> an ODR violation as any other part of the code.

In the present code, we violate the ODR *before* being able to assess if that’s 
an ODR violation we are OK with.

Imagine you check for a null pointer. If you write:

if (ptr)
return ptr->foo();
else
return 0;

that’s OK, because you test the pointer before using it. There is no undefined 
behavior in C++. However, if instead you write:

int Foo::foo()
{
if (this)
…
else
return 0;
}

then there is a violation of C++ semantics. It may work with some compilers, 
some other compilers will optimize away the ‘if (this)’ test (I believe clang 
does).

Our current version check looks like the second example, i.e. the test is done 
too late, so that at the point where we test, we violate C++ semantics and the 
test does not yield the expected results. In the most severe case, the test in 
today’s code would crash instead of detecting the version check mismatch.


> Yet it's singled out as an ODR violation that has to be fixed in the commit 
> log.

It’s singled out because the binary-compatibility check should not itself rely 
on the code being binary compatible :-) So it’s different in that way.

For later ODR violations, it’s OK if the code “trusts” our binary compatibility 
analysis, whatever that analysis may be. There is no known automated way to 
detect an ODR violation in the general case, otherwise, compilers would do it…


> By reading it, yes I kind of expect that we are no longer having similar 
> violations anywhere in the code, otherwise we would fix them too.

As I stated in my previous reply, the mechanism makes it *possible* for us to 
entirely avoid later ODRs. But thats’ a *policy* that you can enforce with the 
mechanism. It’s a choice. The trade-off, as I also explained in my previous 
reply, is that if you stick to the strictest ODR policy, then practically any 
change in the Agent class will be marked as binary incompatible. That policy 
would be fine with me, btw, but with g++, it’s probably a bit too paranoid, and 
not what Frediano for example initially intended.

In any case, could you suggest a wording that you think would be less 
“misleading”?


> Imo what really matters here is that we need this check to detect ABI 
> changes. which are problematic from an ODR point of view, it's not the ODR 
> violation by itself. But it really is just a matter of wording.

The existing initial ODR violation is problematic inasmuch as its primary 
objective is to assess if potential ODR violations are acceptable (“binary 
compatible”) or not (“binary incompatible”).

Anyway, a better wording is welcome, as long as it is not less precise or 
correct than the current one. Since I do not fully understand what you find 
misleading in it, I cannot offer a suggestion.

Also, let’s make sure we are not once more spiralling into fruitless 
bikeshedding. If the patch is correct and fixes an actual bug, and if the 
wording is not blatantly incorrect (i.e. after fixing the spelling mistakes 
Frediano pointed out), we might want to get the fix in instead of arguing for 
days about it.


Christophe “had successfully avoided wearing a C++ lawyer hat for nearly 18 
years, and was not too unhappy about it"

> 
> Christophe

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] gstreamer for view/record spice session

[Snir Sheriber]

Hi,


On 03/28/2018 08:25 PM, Krutskikh Ivan wrote:

Hi!

Is it possible ro use gstreamer to view or record spice screen session?



I'm not sure if i understood what you meant, can you elaborate?

Anyway maybe one of these options will work for you:

-Record spice server traffic and then replay it using spice replay tool
 see user manual:
 https://www.spice-space.org/spice-user-manual.html

-Record your screen inside the guest or recording your client's window
 using Gstreamer and save it into a video file (but it has nothing to 
do with

 spice.

Snir.



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check

[Christophe Fergeau]

On Wed, Mar 28, 2018 at 05:54:21PM +0200, Christophe de Dinechin wrote:
> > With that said, I find the current "ODR" portion of the commit log
> > misleading, it's easy to get the impression that after this change, we
> > won't have any ODR problem anymore (this is what I initially thought!).
> 
> Why do you find it misleading?
> 
> The precise wording is "The current plugin version check violates the
> C++ ODR”. It specifically talks about “current plugin version check”.
> And there is indeed no ODR violation in the version check after that
> change.
> 
> Whether there are ODR violations elsewhere, and whether we accept such
> violations purposefully on a case by case basis for other calls is a
> judgement call. The mechanism makes it entirely possible to avoid ODR
> violations completely (at the cost of unnecessarily restricting binary
> compatibility).
> 
> So I don’t see it as misleading or implying that it magically gets rid
> of other ODR violations.

As long as we don't change the ABI, the version check is as much
an ODR violation as any other part of the code. Yet it's singled out
as an ODR violation that has to be fixed in the commit log. By reading
it, yes I kind of expect that we are no longer having similar violations
anywhere in the code, otherwise we would fix them too. Imo what really
matters here is that we need this check to detect ABI changes. which are
problematic from an ODR point of view, it's not the ODR violation by
itself. But it really is just a matter of wording.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 0/2] Make plugin version checking more robust

[Christophe de Dinechin]

> On 29 Mar 2018, at 09:14, Christophe de Dinechin  wrote:
> 
> 
> 
>> On 28 Mar 2018, at 18:46, Christophe Fergeau  wrote:
>> 
>> On Wed, Mar 28, 2018 at 06:06:19PM +0200, Christophe de Dinechin wrote:
 If my task is to "move version check to the agent", do I _have_ to change
 the semantics of the version check? No.
>>> 
>>> Of course you have to. There is no “PluginVersionIsCompatible”
>>> anymore, etc, so the version number semantics have changed whether you
>>> like it or not. You may artificially try to make the new version
>>> number look like the old one, and I would have if there wasn’t another
>>> problem with that numbering.
>> 
>> Yes, "another problem", which is why it's much better if we split them...
>> https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/
> 
> Which I will quote, then:
> 
>   • Mixing two unrelated functional changes. Again the reviewer will find 
> it harder to identify flaws if two unrelated changes are mixed together. If 
> it becomes necessary to later revert a broken commit the two unrelated 
> changes will need to be untangled, with further risk of bug creation.
> 
> I underline “unrelated”. I have proven that the changes were unrelated, and 
> so did your own attempt at splitting, which require confusing and/or 
> bug-introducing changes to the same piece of code.

*not* unrelated, obviously…

> 
>> 
>> This also makes the review process more complicated, as one has to
>> figure out what part of the patch is meant to achieve what. In this
>> case, I'd be fine ACK'ing the first 2 changes, but I haven't given much
>> thought regarding the versioning yet.
> 
> Maybe you should give it some thought then, instead of immediately jumping to 
> conclusions and demanding that the patch be split.
> 
> 
> Thanks
> Christophe
> 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 0/2] Make plugin version checking more robust

[Christophe de Dinechin]

> On 28 Mar 2018, at 18:46, Christophe Fergeau  wrote:
> 
> On Wed, Mar 28, 2018 at 06:06:19PM +0200, Christophe de Dinechin wrote:
>>> If my task is to "move version check to the agent", do I _have_ to change
>>> the semantics of the version check? No.
>> 
>> Of course you have to. There is no “PluginVersionIsCompatible”
>> anymore, etc, so the version number semantics have changed whether you
>> like it or not. You may artificially try to make the new version
>> number look like the old one, and I would have if there wasn’t another
>> problem with that numbering.
> 
> Yes, "another problem", which is why it's much better if we split them...
> https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/

Which I will quote, then:

• Mixing two unrelated functional changes. Again the reviewer will find 
it harder to identify flaws if two unrelated changes are mixed together. If it 
becomes necessary to later revert a broken commit the two unrelated changes 
will need to be untangled, with further risk of bug creation.

I underline “unrelated”. I have proven that the changes were unrelated, and so 
did your own attempt at splitting, which require confusing and/or 
bug-introducing changes to the same piece of code.

> 
> This also makes the review process more complicated, as one has to
> figure out what part of the patch is meant to achieve what. In this
> case, I'd be fine ACK'ing the first 2 changes, but I haven't given much
> thought regarding the versioning yet.

Maybe you should give it some thought then, instead of immediately jumping to 
conclusions and demanding that the patch be split.


Thanks
Christophe

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

[Christophe de Dinechin]

> On 7 Mar 2018, at 12:51, Christophe Fergeau  wrote:
> 
> On Wed, Mar 07, 2018 at 12:28:57PM +0100, Christophe de Dinechin wrote:
>>> This behaviour seems inconsistent with std::runtime_error(const char *)
>>> which as far as I can tell does not have this lifetime expectation.
>> 
>> You are correct, but please note that std::runtime_error takes a string as 
>> input argument, making it very clear that you are dealing with a ‘string’ 
>> underneath, not a const char*.
> 
> Nope, it accepts both in c++11

I know that. (Quizz: can you explain why?)

The point was that it has a string constructor. So you don’t get an error if 
you use a string as an argument.

> http://www.cplusplus.com/reference/stdexcept/runtime_error/
> And the const char * version will make a copy of the const char * you
> give it...
> 
>> 
>>> 
 
 Anyway, your example is misleading. Anybody that passes the result of
 string::c_str() to a const char * without proper consideration for the
 lifetime is at fault. The const char * interface itself cannot be
 faulted for that.
>>> 
>>> For what it's worth, I usually expect const char * interfaces to make
>>> their own copy of the const char * arg if they need it after they
>>> return, and I've rarely seen exceptions to this expectation.
>> 
>> std::vector and all standard library containers? You do a push_back of a 
>> const char * in an std::vector, you get a copy. If you do that in an 
>> std::vector, you don’t.
> 
> I'd argue templated code is special.

Why?

(I can probably find non-template examples, but I’d like to understand your 
reasoning first)

> 
>> Let me give an example from *our own code*. ConcreteConfigureOption
>> takes two const char * arguments. Does it make copies? No. Same thing
>> with the related AddOption method. Anything particularly surprising
>> there? I don’t think so.
> 
> Hmm, in my opinion it should make a copy... Given what it's used for,
> it's highly likely it's going to be used with static strings, so why
> not.

What makes options more “likely” to be used with static strings than the Error 
constructor? We are talking about a class designed to store error messages. 
Anything more static than that?


> But I really would not use that as a generic example which should
> be followed throughout the codebase ;)

I am not. I am just pointing out that your expectations are not met in our own 
codebase.


> 
>>> If I understood properly, things are designed this way to avoid
>>> running out of memory while building the exception. I'm definitely
>>> not in favour of having an error-prone API to handle a very rare
>>> case which the rest of the code is most likely not going to be able
>>> to cope with anyway.
>> 
>> To make this clear, let me elaborate a little on *one* other reason (I
>> listed a few others), code bloat and runtime cost. Let’s say we want
>> to store a write error with an errno. The two approaches under
>> consideration are:
>> 
>>  A. throw WriteError(message, operation, errno);
>>  B. throw std::runtime_error(message + “ in “ + operation + “: “ + 
>> strerror(errno))
>> 
>> I’ll briefly mention, as a gentle reminder of some of the “other reasons”, 
>> that (A) is
>> 1. shorter to type
>> 2. easier toi read
>> 3. less error prone
>> 4. more likely to guarantee consistent messages
> 
> No problem with having A as an API, but I don't think achieving A
> mandates that we don't generate the string at the time the exception is
> created. This really is what is being discussed at this point.

Well, I’m glad I convinced you so far, it was not quite clear until now. I will 
stop arguing that then.

So then my next question is: if there is a message formatting approach that 
does not require dynamic memory allocation nor throws exceptions, why settle 
for less?

> And if we are optimizing exception creation from the start, without any
> profiling data saying we have to do this, I'd say we are falling into
> very premature optimizations…

From my point of view, you are requesting a large number of severe belated 
pessimizations. Belated because I already did the optimization, and you are 
asking me to remove it after having actually implemented it. Pessimizations 
because you are asking to make the code worse, and you seem to be aware of 
that. It’s often said that “premature optimization is the root of all evil”. 
Len Lattanzi liked to point out that “belated pessimization is the leaf of no 
good”.

Anyway, I could go with de-optimizing if we gained something from it, if it was 
an actual trade-off. But here, there is no trade-off. It’s replacing a win-win 
with a lose-lose. In addition to making the code less efficient, it would 
encourage programmers to pass dynamic strings. This is an Error class. Its 
message are by definition error messages. They are supposed to be static 
constants, by design. String concatenation in error messages is widely 
acknowledged as evil.

I could have selected a different