-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Improve C/C++ preprocessing #369
base: master
Are you sure you want to change the base?
Conversation
Hi @jdehaan this is real cool, thanks. I see the new implementation not only pass the old tests but also works for the 4 new scenarios. Great! The call method is a bit too long and the CCN is quite high, I would say. Do you mind refactor it a bit? |
Yes @terryyin I am still a bit unsure if that is really a good idea to skip the preprocessor information, it is code anyway maybe dead code on the one or other side of the #if's and #elif's but in the end this could be the beginning of a better or more thorough handling of such "switchable" code parts. To really know which parts are "on" or "off" could be a long term goal. For the moment you are fully right I will try to simplify the code as much as possible to provide a clear and extensible code for further future improvements. |
@terryyin I simplified by splitting the code into its essential parts so future extensions can be added by not having to tweak a single spaghetti code central method. There is an intrinsic complexity to the problem itself, though. I think I am close to it. Can you please review again and tell me if that is ok or drive me in some direction you want the code the get to. |
BTW The AppVeyor CI complains about python 2.7, but honestly is that still a valuable topic to be checked? Python3 is there since a very long time and I see no good reason to forcibly have to support 2.7. |
It also sounds like there's now a basic preprocessing step in place that can skip or take a branch based on certain conditions. For example, the #if 0 statement can be used to disable certain sections of the code explicitly. That can be really useful when you need to test different parts of your code or disable certain features temporarily. Have you had a chance to try out these changes yet? |
No not yet, I was not aware of that. I saw that there was an extension for a purpose that sounded like a use case I need and noticed it biased the analysis and is a little bit better with this PR. But in the very end I am unsure how to treat this "correctly". Slowly I recognize there is code we do not care at all about, dead code (#if 0), that sometimes we really also want a holistic report (notwithstanding if that is actually landing into a compiled binary or not)... Also in the case we collect all the metrics there can be function names duplicated which sometimes matter in the later processing steps (after collecting lizard metrics). So having a good way - beside always varying line numbers - to associate the code with some identifier would be a "must-have". To address all the use cases, after thinking more about it, it would be good to annotate the functions with the preprocessor conditions leading to it to be "included". Special handling for the Then it could be up to a post-processing done outside of lizard's scope to filter out unwanted things... This is a tedious task but might be rewarding. But this is nothing I would like to takle on my own before having a discussion with the author's opinion or what are the things foreseen for lizard'S roadmap regarding these aspects... @terryyin what do you think? In the meanwhile I still think this PR provided a slightly improved situation but far beyond of what I would consider being an ideal state... :-/ PS: As the tool supports multiple programming language, we might have to consider how the activation/deactivation of lines for a translation unit in C/C++ via preprocessor switches applies maybe to other programming languages as well... |
-Ecpre
is replaced with a more explicit-Ecpreprocessor
#if 0
leads to that part to be skipped. This is often found in code to disable sections explicitly.