Skip to content

Comments

removed duplicated code by combining conditions in #ifdef directives#411

Draft
mi-hol wants to merge 1 commit intoespotek-org:masterfrom
mi-hol:master
Draft

removed duplicated code by combining conditions in #ifdef directives#411
mi-hol wants to merge 1 commit intoespotek-org:masterfrom
mi-hol:master

Conversation

@mi-hol
Copy link
Contributor

@mi-hol mi-hol commented Feb 15, 2026

also make conditional logic in preprocessor directives (i.e for PLATFORM_ definitions) better understandable by indenting them to level of surrounding code.

Note: no functional changes!

@mi-hol
Copy link
Contributor Author

mi-hol commented Feb 15, 2026

@turboencabulator @mmehari I'd appreciate a quick feedback if these changes make the code better readable from your view.
Note: this is just a proof of concept and not finalized yet!

@mmehari
Copy link
Contributor

mmehari commented Feb 15, 2026

I would prefer to keep it the way it was since there are IDEs which grey out the not used code and it appears as if it was written without the directives.

@mi-hol
Copy link
Contributor Author

mi-hol commented Feb 15, 2026

there are IDEs which grey out the not used code and it appears as if it was written without the directives.

I'm not following, would you perhaps have a screenshot of what you tried to explain?

@mmehari
Copy link
Contributor

mmehari commented Feb 16, 2026

Here is a screenshot from Qt Creator when viewing mainwindow.cpp file in the editor page.

image

Since I am working on a Linux machine, those directives related to Windows are grayed out and wouldn't bother if kept without being indented.

@mi-hol
Copy link
Contributor Author

mi-hol commented Feb 16, 2026

screenshot from Qt Creator

@mmehari thanks, that triggers new questions.

  1. What version of QT Creator do you use?
  2. How do the lines 58/following look in QT Creator before and after the change?

@mmehari
Copy link
Contributor

mmehari commented Feb 16, 2026

screenshot from Qt Creator

@mmehari thanks, that triggers new questions.

  1. What version of QT Creator do you use?

Qt Creator 18.0.2

  1. How do the lines 58/following look in QT Creator before and after the change?

Before
image

After
image

@mi-hol
Copy link
Contributor Author

mi-hol commented Feb 17, 2026

Since I am working on a Linux machine, those directives related to Windows are grayed out and wouldn't bother if kept without being indented.

The screenshots leave me confused, because from my view the behaviour of QT Creator is the same for indented and non-indented directives.
But users of other editors benefit from the easier visiual destinction of conditional code identified by indents.
Do I miss something ?

@mmehari
Copy link
Contributor

mmehari commented Feb 17, 2026

Yes, the behaviour is the same since QT Creator only greys out the code that will not be compiled.

You didn't miss anything and I understand your point of view which at the end is a matter of preference.

I am OK with your changes as well.

@mi-hol
Copy link
Contributor Author

mi-hol commented Feb 17, 2026

which at the end is a matter of preference

From my view it's much more than personal preference because it "can reap significant improvements in comprehension and productivity". Reasoning is well explained in https://thelinuxcode.com/why-you-should-use-column-indentation-to-improve-your-codes-readability/
Maybe that explains why I bring such topics up and invest my time there.

@mmehari
Copy link
Contributor

mmehari commented Feb 18, 2026

I think it is getting a bit out of context.

For the record, I am in favor of indentation and I am not saying otherwise. But specific to this situation, pre-processor directives are resolved before compilation and people tend to do the indentation at this level. Which is why IDE editors grey them out and the reason for my preference.

@EspoTek
Copy link
Collaborator

EspoTek commented Feb 19, 2026

I'm with @mmehari on this one too.

It seems strange to me to indent an #ifdef, since it doesn't compile to anything and isn't a part of the regular code.

Preprocessor stuff starting at column 0 seems to be the style that Atmel use too:
https://github.com/espotek-org/Labrador/blob/master/AVR_Code/USB_BULK_TEST/src/ASF/common/services/usb/udc/udc.c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants