split the actual check implementations from the instances

[firewave]

No description provided.

[firewave]

A side note - outside of the tests we would actually not require any of the checks to have headers at all since the application code is only accessing them through the instances.

[firewave]

The check objects were ambiguous in their purpose. The constructor with the name only constructed the instance which had all internal pointer set to NULL and should never be used to run the checks. The other constructor was used to generate the actual object for the analysis. This splits the checks into separate instance and implementation classes.

This will allow us to change the pointer to references (a follow-up I have already lined up) and also clean up some interfaces (more follow-ups).

There is most likely some additional cleanups possible with the access level of the implementations (non-analysis function should probably not be public - well, actually none of them should probably be public in the production code but I would not like to introduce friend declarations as that would mess with the unused function detection - something to re-visit later on - see also below).

A side note - outside of the tests we would actually not require any of the checks to have headers at all since the application code is only accessing them through the instances.

This is no longer the intended goal. The idea was to hide the implementation completely and only use the interface. But with umbrella checks like CheckOther that doesn't work because we do not want to run unit tests as a whole on those but only specific checks. Actually we should be doing this for all unit tests and create the *Impl and only call the checks we actually want to test (that is a further follow-up).

[danmar]

I know you hate AI but it's hard to review such big change manually. AI has this feedback:

The Impl suffix is universally associated with the PIMPL (pointer-to-implementation) pattern, where FooImpl is a hidden private type
owned by Foo. Here, CheckAssertImpl is completely unrelated to CheckAssert via inheritance — it is a sibling, not a detail. This will
mislead contributors who see CheckAssertImpl and look for a CheckAssert wrapper owning it. Consider CheckAssertRunner, or keeping the original name for the implementation class and using CheckAssertReg for the
registration stub

Any opinions about this? Personally I have never used PIMPL. So I am not confused..

[danmar]

Another question:

2. getErrorMessages() is still in Check*, not CheckImpl
   The error-message strings are defined in the same methods that call reportError(). But reportError() now lives in CheckImpl. For getErrorMessages(), a null ErrorLogger trick was historically used — with the split, does Check*::getErrorMessages() instantiate a Check*Impl internally? If not, how is reportError() reached? This needs to be clearly shown in the PR or documented; it's a non-obvious dependency.

[firewave]

Any opinions about this? Personally I have never used PIMPL. So I am not confused..

It is just called *Impl - we are not using PIMPL here.

with the split, does Check*::getErrorMessages() instantiate a Check*Impl internally?

Obviously. The code in those function has not changed at all - we just split the instance code from the implementation one so the Check* objects are no longer ambiguous in what they can be used for.