fix #13021: cmdline: validate option --std

[ludviggunne]

No description provided.

[firewave]

Err, no. This adds additional knowledge to the code.

Leveraging the existing logic would be to bail out when the result of the get{C|CPP}() call is *Latest.

The next step would be to leverage the logic from simplecpp in those calls. Maybe moving the Standards enum to that.

[danmar]

Leveraging the existing logic would be to bail out when the result of the get{C|CPP}() call is *Latest.

hmm no.. if user writes "c++26" then getCPP() will return CPPLatest.

[ludviggunne]

Err, no. This adds additional knowledge to the code.

Leveraging the existing logic would be to bail out when the result of the get{C|CPP}() call is *Latest.

The next step would be to leverage the logic from simplecpp in those calls. Maybe moving the Standards enum to that.

Ok sorry, must have misunderstood. I don't think it would be enough to check the return value of getC/getCPP since CPPLatest == CPP26 (--std=c++26 would get rejected). Maybe I should add back the bool reference approach from the last attempt?

[danmar]

but assuming that std is the standard you can write:

 if (getCPP(getCPP(std)) == std) {
      /* valid c++ standard */
 }

EDIT: To clarify this leverages both getCPP(const std::string &std) and getCPP(cppstd_t std)

[ludviggunne]

but assuming that std is the standard you can write:

 if (getCPP(getCPP(std)) == std) {

      /* valid c++ standard */

 }

EDIT: To clarify this leverages both getCPP(const std::string &std) and getCPP(cppstd_t std)

Sounds good, I'll do that.

[firewave]

hmm no.. if user writes "c++26" then getCPP() will return CPPLatest.

Ah - right. I haven't changed that yet that the latest enum has a different meaning. Or did I try that already and did not want to do it for some reason?

I guess that's why I did not fix that TODO yet since it as a ripple effect - as usual.

but assuming that std is the standard you can write:

 if (getCPP(getCPP(std)) == std) {
      /* valid c++ standard */
 }

EDIT: To clarify this leverages both getCPP(const std::string &std) and getCPP(cppstd_t std)

Also thought about the roundtrip approach but the other seemed cleaner.

But that won't work either because CPPLatest == CPP26. I guess currently we need to (ab)use Standards::setC() and Standards::setCPP() as the validation check.

[danmar]

But that won't work either because CPPLatest == CPP26.

then tell me why.

• If std is "unknown" then the first getCPP() returns CPPLatest. The second getCPP() returns "c++26". The condition "c++26" == std will be false.
• If std is "c++26" then the first getCPP() returns CPP26. The second getCPP() returns "c++26". The condition "c++26" == std will be true.

[firewave]

But that won't work either because CPPLatest == CPP26.

then tell me why.

Sorry - I missed the comparison in the if-condition.

But that is essentially the code we have in those set*() functions. So no need to replicate that.

[ludviggunne]

I have a draft that just runs the getC/getCPP on the mSettings.standards instance, which I think is pretty clean. Just have to run through the tests.

[firewave]

Using the set* functions is much cleaner

if (!mSettings.standards.setC(std) && !mSettings.standards.setCPP(std)) {
// error
}

[ludviggunne]

Using the set* functions is much cleaner

if (!mSettings.standards.setC(std) && !mSettings.standards.setCPP(std)) {
// error
}

Yes this is true, changed it.

[ludviggunne]

Is the next step to expose a standards enum in simplecpp? Is there a specific reason why cppstd_t and cstd_t are kept separate?

[firewave]

Is the next step to expose a standards enum in simplecpp?

Maybe!??!?

Is there a specific reason why cppstd_t and cstd_t are kept separate?

Apple and Oranges. It makes things awkward when coming from the outside - but looking from the inside out it makes a lot of sense. It is possible this has to be looked into when we have we have proper testing of the handling in the project imports and when merging FileWithDetails and FileSettings.

I think I just realized an issue with this change.

Currently --std=c++11 --std=c23 will set both of these standards. With the changes that invalidates the previously set one. So we need to use a temporary Standards object for validation and only apply it to the actual settings if it is valid. So add that object, split the set calls and fetch the standard to set from that. Also please add some test cases for this. Also need to test that things are properly overwritten.

[danmar]

Currently --std=c++11 --std=c23 will set both of these standards.

Good catch.. yes we want to allow that and if user writes that then the cpp standard should be set to c++11 and the c standard should be set to c23.

[firewave]

FYI I opened #6742 which adds some adjustments, tests and TODOs for things we will encountered in the next step.

[danmar]

I just have a small nit

[danmar]

If CI is happy I believe we can merge this.

[danmar]

@ludviggunne if you feel this can be merged then convert it to a regular Pr (clicking on the "Ready for review" button). For me it looks OK now.

[firewave]

LGTM.

Thanks for your contribution and baring with the all feedback.

[ludviggunne]

No worries, it was just nice to get a slightly broader insight in to the project.

[firewave]

No worries, it was just nice to get a slightly broader insight in to the project.

Some of it ain't pretty...yet.

[firewave]

This broke the forwarding of --std when using --clang because it does not set mSettings.standards.stdValue.

It will implicitly fixed with https://trac.cppcheck.net/ticket/13129.

That's why I sometimes push back hard on even the simplest looking fixes because some parts still lack even basic test coverage and are quite fragile.

Fortunately --clang should not have many users (it is still experimental and rather unusable).