Remove the debug defines when building with RelWithDebInfo

[wawanbreton]

The variables DEBUG, USE_CPU_TIME and ASSERT_INSANE_OUTPUT were defined also when building in mode RelWithDebInfo, which is used when building the production installers. This leads to some debug code being executed by customers, with potential crashes and decreased performances.

Contributes to CURA-11821

[rburema]

So are we building in that mode to get the(/more useful) Sentry info?

I'm not about to request a change, since I approve (regardless of Sentry), because it's nice to have semi-debug-able release code (especially since we're OSS and therefore have little to hide if anything), but it does make me wonder what prompted us to do that in the first place.

[rburema]

Now that I'm testing this, since we often have a large output, maybe it'd be a good idea to print it at the end as well (not instead, because of crashes, just 'as well').

[jellespijker]

So are we building in that mode to get the(/more useful) Sentry info?

I'm not about to request a change, since I approve (regardless of Sentry), because it's nice to have semi-debug-able release code (especially since we're OSS and therefore have little to hide if anything), but it does make me wonder what prompted us to do that in the first place.

One word: "Windows"

That was the only way we could get sensible stack changes for 70% of our users.

[wawanbreton]

Now that I'm testing this, since we often have a large output, maybe it'd be a good idea to print it at the end as well (not instead, because of crashes, just 'as well').

That's a good idea, so that you're quite sure not to miss it. But it may be a bit annoying in practice, because we (I) often look at the output once it has run, to see if it ended properly or if there is something important to note. With that, we would only see the message each time and have to go up to see anything interesting. So, not sure.

[rburema]

Now that I'm testing this, since we often have a large output, maybe it'd be a good idea to print it at the end as well (not instead, because of crashes, just 'as well').

That's a good idea, so that you're quite sure not to miss it. But it may be a bit annoying in practice, because we (I) often look at the output once it has run, to see if it ended properly or if there is something important to note. With that, we would only see the message each time and have to go up to see anything interesting. So, not sure.

Yes, the same thing that makes the 2nd message useful is also the one that'll make it annoying 😅 I can see that. -- And we'd only have to check once for each release build anyway 🤔 -- so we can probably do without the 2nd message and only add it if it ever becomes a problem not to.

[wawanbreton]

And we'd only have to check once for each release build anyway 🤔

Not even sure we have to actively check it. It is here more as a safety, like when you get a user log file. If the message is present, you won't miss it and know directly that something is wrong.

[wawanbreton]

I'm going to approve, since I think the message is clear either way, but both this and the tree suffer from the fact that ASCII is only properly defined for the 1st 128 characters instead of the full 256 ... the rest are these 'codepages' that can be different.

Oh that really doesn't look good... If I'm correct, this character is part of the "extended ASCII codes" which I assumed would be universal enough. But maybe a UTF-8 character would be better ? Otherwise I can use a standard ASCII character, it will just be slightly less nice :smiling_face_with_tear:

[rburema]

If I'm correct, this character is part of the "extended ASCII codes" which I assumed would be universal enough.

The problem is that 'extended ASCII' can mean a number of things 😓

According to wikipedia:

There is no formal definition of "extended ASCII", and even use of the term is sometimes criticized,[1][2][3]

I think either solution is fine. (Note that this also affects the otherwise nice tree-representation-output you made for another purpose in the engine -- that isn't quite as disruptive as this and wasn't meant as 100% cosmetic, so I didn't mention it.)

[wawanbreton]

There is no formal definition of "extended ASCII", and even use of the term is sometimes criticized,[1][2][3]

Well I learned something then, I thought thoses codes were standard.

I think either solution is fine. (Note that this also affects the otherwise nice tree-representation-output you made for another purpose in the engine -- that isn't quite as disruptive as this and wasn't meant as 100% cosmetic, so I didn't mention it.)

Nobody mentioned that the tree representation was faulty, so I assumed it worked well. I'll give it a try with UTF-8 characters.

[rburema]

Nobody mentioned that the tree representation was faulty, so I assumed it worked well. I'll give it a try with UTF-8 characters.

Sorry, it kept slipping my mind 😓