Revert "Fix check for cpp macros and usage"
ClosedPublic

Authored by christiand on Jan 28 2018, 6:10 PM.

Details

Reviewers
wojnilowicz
pino
Summary

The original commit intended to fix a warning by our code checker krazy.
The issue the code checkers criticized is that we use code which is
only understand by a single compiler and thus not portable.
However, even the reverted commit uses the non portable code but is
hiding the issue from the checker. Therefore it is not a real fix.

Also the reverted commit introduces project specific knowledge,
the IS_GNU macro which has more general replacements (GNUC).

Additionally other compilers than GNU's GCC may have support for the
used attribute (I thing clang and Intel have). These compilers were
excluded from using the special behaviour.

This reverts commit 8f7c5ce861fc3d9fe6b6e59a24da3249ce91cf88.

Diff Detail

Repository
R261 KMyMoney
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
christiand requested review of this revision.Jan 28 2018, 6:10 PM
christiand created this revision.

I have some questions for that:

  1. As I see correctly, that brings us warning by our code checker krazy back. What for?
  2. As I see correctly, IS_GNU is arbitrary variable created by me and not any macro. Why do you call it a macro?
  3. Do you say "if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")" isn't portable? It's standard CMake variable, so why isn't it portable?

Before anything with this patch, I would like to know if it compiles on MS Windows.

I have some questions for that:

  1. As I see correctly, that brings us warning by our code checker krazy back. What for?

Yes the warning in ebn.kde.org will be back but it is probably not correct in this case (also see below). Krazy gives hints but can be wrong, too. The benefit is simplified code and the other benefits written before. If you want I can recheck the ebn warnings and see if the warning is correct.

  1. As I see correctly, IS_GNU is arbitrary variable created by me and not any macro. Why do you call it a macro?

It is a variable in CMake context, over #cmakedefine IS_GNU 1 in config-kmymoney.h.cmake you made it marco in C++. For this reason you were able to use #ifdef IS_GNU.

  1. Do you say "if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")" isn't portable? It's standard CMake variable, so why isn't it portable?

No, the CMake part is portable. The non-portable part is __attribute__((format(__printf__, x, y))). Was the warning "C/C++ source files and non-installed headers should NOT use cpp conditionals that check for a certain O/S or compiler; instead use HAVE_foo macros. We want to check for features discovered during configure-time rather than for a specific O/S."? If so, the CMake code did not resolve the issue, it just got hidden from klazy (because it checks the compiler not the feature). The __GNUC__ macro is actually the first choice here. Because if it is defined, the compiler indicates that it supports the __attribute__ syntax. Note that several compilers define __GNUC__ because they support this (they were excluded by the CMake based check).

Before anything with this patch, I would like to know if it compiles on MS Windows.

I think so. This is not an operating system depending code but compiler specific. I never compiled KMyMoney on Windows.

I have some questions for that:

  1. As I see correctly, that brings us warning by our code checker krazy back. What for?

Yes the warning in ebn.kde.org will be back but it is probably not correct in this case (also see below). Krazy gives hints but can be wrong, too. The benefit is simplified code and the other benefits written before. If you want I can recheck the ebn warnings and see if the warning is correct.

I agree that code will be simplified and that krazy could give wrong hints but I don't wan't to see warnings with it and I don't want to question krazy correctness.

Could you send a bug to krazy repository at github?
I've once done so here
https://github.com/Krazy-collection/krazy/issues/6
and the response was quick.
If your questioning of krazy correctness will be accepted by krazy maintainer Allen Winter, then I am in for this patch.

  1. As I see correctly, IS_GNU is arbitrary variable created by me and not any macro. Why do you call it a macro?

It is a variable in CMake context, over #cmakedefine IS_GNU 1 in config-kmymoney.h.cmake you made it marco in C++. For this reason you were able to use #ifdef IS_GNU.

  1. Do you say "if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")" isn't portable? It's standard CMake variable, so why isn't it portable?

No, the CMake part is portable. The non-portable part is __attribute__((format(__printf__, x, y))). Was the warning "C/C++ source files and non-installed headers should NOT use cpp conditionals that check for a certain O/S or compiler; instead use HAVE_foo macros. We want to check for features discovered during configure-time rather than for a specific O/S."? If so, the CMake code did not resolve the issue, it just got hidden from klazy (because it checks the compiler not the feature). The __GNUC__ macro is actually the first choice here. Because if it is defined, the compiler indicates that it supports the __attribute__ syntax. Note that several compilers define __GNUC__ because they support this (they were excluded by the CMake based check).

It seems then, that this was a workaround to silence krazy warning, but how else could we make krazy happy, if not changing how krazy works?

Before anything with this patch, I would like to know if it compiles on MS Windows.

I think so. This is not an operating system depending code but compiler specific. I never compiled KMyMoney on Windows.

I could check that by occasion if it's not urgent.

If your questioning of krazy correctness will be accepted by krazy maintainer Allen Winter, then I am in for this patch.

Then I suggest to apply this patch, see what ebn says and then check if we should forward it upstream.

It seems then, that this was a workaround to silence krazy warning, but how else could we make krazy happy, if not changing how krazy works?

With //krazy:exclude=... krazy becomes relaxed :)

Before anything with this patch, I would like to know if it compiles on MS Windows.

I think so. This is not an operating system depending code but compiler specific. I never compiled KMyMoney on Windows.

I could check that by occasion if it's not urgent.

It is not urgent, I can wait until 5.0 us out.

If your questioning of krazy correctness will be accepted by krazy maintainer Allen Winter, then I am in for this patch.

Then I suggest to apply this patch, see what ebn says and then check if we should forward it upstream.

Please apply, but only with intention to get rid of ebn warning afterwards.

It seems then, that this was a workaround to silence krazy warning, but how else could we make krazy happy, if not changing how krazy works?

With //krazy:exclude=... krazy becomes relaxed :)

I think it's not the best way either and that we should fix every 'krazy:exclude='

Before anything with this patch, I would like to know if it compiles on MS Windows.

I think so. This is not an operating system depending code but compiler specific. I never compiled KMyMoney on Windows.

I could check that by occasion if it's not urgent.

It is not urgent, I can wait until 5.0 us out.

pino accepted this revision.Feb 8 2018, 8:29 AM
pino added a subscriber: pino.

The change is correct, commit 8f7c5ce861fc3d9fe6b6e59a24da3249ce91cf88 just pleased a non-smart tool.
This is a bug in krazy, so please report it there, instead of working it around in kmymoney.

This revision is now accepted and ready to land.Feb 8 2018, 8:29 AM
wojnilowicz closed this revision.May 7 2018, 1:59 PM

I reverted it as a byproduct of D12737.

I will change krazy to not complain about GNUC

I will change krazy to not complain about GNUC

Thank you :)