Skip to content

Allow indenting preprocessor macros? #1006

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

Closed
matthijskooijman opened this issue Mar 22, 2020 · 6 comments · Fixed by #1007
Closed

Allow indenting preprocessor macros? #1006

matthijskooijman opened this issue Mar 22, 2020 · 6 comments · Fixed by #1007

Comments

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Mar 22, 2020

While developing #997, I wrote some complicated and nested preprocessor statements. I originally tried making them clearer by applying indents, but that was not allowed by the astyle checks, so I removed them again. Now I wonder, should this astyle rule maybe be changed to allow (even if not require) indenting preprocessor directives?

e.g. consider:

#if defined(USBD_ATTACH_PIN)
#define USBD_PULLUP_CONTROL_PINNAME digitalPinToPinName(USBD_ATTACH_PIN)
#define USBD_DETACH_LEVEL !(USBD_ATTACH_LEVEL)
#elif defined(USBD_DETACH_PIN)
#define USBD_DETACH_PINNAME digitalPinToPinName(USBD_DETACH_PIN)
#define USBD_ATTACH_LEVEL !(USBD_DETACH_LEVEL)
#elif !USBD_HAVE_INTERNAL_PULLUPS
/* When no USB attach and detach pins were defined, and the USB module
 * also does not have any builtin pullups, assume there is a fixed
 * external pullup and apply the D+ trick. */
#if defined(USE_USB_HS_IN_FS)
#define USBD_PULLUP_CONTROL_PINNAME USB_OTG_HS_DP
#elif defined(USB_OTG_FS)
#define USBD_PULLUP_CONTROL_PINNAME USB_OTG_FS_DP
#else /* USB */
#define USBD_PULLUP_CONTROL_PINNAME  USB_DP
#endif
/* Detach by writing LOW, but leave floating instead of writing
 * HIGH to attach. */
#define USBD_DETACH_LEVEL LOW
#define USBD_PULLUP_CONTROL_FLOATING
#endif /* !defined(USBD_ATTACH_PINNAME) && !USBD_HAVE_INTERNAL_PULLUPS */

vs:

#if defined(USBD_ATTACH_PIN)
    #define USBD_PULLUP_CONTROL_PINNAME digitalPinToPinName(USBD_ATTACH_PIN)
    #define USBD_DETACH_LEVEL !(USBD_ATTACH_LEVEL)
#elif defined(USBD_DETACH_PIN)
    #define USBD_DETACH_PINNAME digitalPinToPinName(USBD_DETACH_PIN)
    #define USBD_ATTACH_LEVEL !(USBD_DETACH_LEVEL)
#elif !USBD_HAVE_INTERNAL_PULLUPS
    /* When no USB attach and detach pins were defined, and the USB module
     * also does not have any builtin pullups, assume there is a fixed
     * external pullup and apply the D+ trick. */
    #if defined(USE_USB_HS_IN_FS)
        #define USBD_PULLUP_CONTROL_PINNAME USB_OTG_HS_DP
    #elif defined(USB_OTG_FS)
        #define USBD_PULLUP_CONTROL_PINNAME USB_OTG_FS_DP
    #else /* USB */
        #define USBD_PULLUP_CONTROL_PINNAME  USB_DP
    #endif
    /* Detach by writing LOW, but leave floating instead of writing
     * HIGH to attach. */
    #define USBD_DETACH_LEVEL LOW
    #define USBD_PULLUP_CONTROL_FLOATING
#endif /* !defined(USBD_ATTACH_PINNAME) && !USBD_HAVE_INTERNAL_PULLUPS */

The latter is IMHO a lot more readable.

@fpistm
Copy link
Member

fpistm commented Mar 22, 2020

Why not.
I've based the astyle rules on Arduino one.

@matthijskooijman
Copy link
Contributor Author

matthijskooijman commented Mar 22, 2020

I had a look at the astyle docs, the indent-preproc-block option manages this. There seems to be no option to allow, but not require this indentation (but I think this is how astyle works with all options), so enabling this would mean a lot of files need to be updated (but again, astyle automates this).

I just created #1007 to see what this would do.

@matthijskooijman
Copy link
Contributor Author

Hm, a related question: It seems that the github astyle check uses https://github.com/stm32duino/actions/tree/master/astyle-check which duplicates both the checking script as well as the astyle configs from the CI subdirectory in this repo. Why does the check not just run the script from this repo, so any changes to that are automatically applied?

@fpistm
Copy link
Member

fpistm commented Mar 23, 2020

Why does the check not just run the script from this repo, so any changes to that are automatically applied?

First goal is to have an action not dependent to the core. I did not find any "official" action for astyle that's why I've made my own one.
Moreover it is possible to specify a custom astyle-definition.

@fpistm
Copy link
Member

fpistm commented Mar 23, 2020

I've fixed that with eeabfc1

I've relaunched the actions on #1007.
Edit: This always failed because it use the workflow file from the based branch. So need to rebase tour branch to use it.

@fpistm fpistm linked a pull request Mar 31, 2020 that will close this issue
@fpistm
Copy link
Member

fpistm commented Mar 31, 2020

Thanks @matthijskooijman
#1007 merged.

@fpistm fpistm closed this as completed Mar 31, 2020
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 a pull request may close this issue.

2 participants