Skip to content
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

Inconsistent formatting of set command with favour-inlining enabled #18

Closed
PatTheMav opened this issue Mar 24, 2024 · 3 comments
Closed

Comments

@PatTheMav
Copy link

With current master branch of gersemi I noticed that even with favour-inlining enabled some invocations of the set command get split up into multiple lines.

In my case the following invocation is kept on a single line:

set(CCACHE_SUPPORT ON CACHE BOOL "Enable ccache support")

Whereas the following invocation is split into multiple lines:

set(
  uuid_token_lengths
  8
  4
  4
  4
  12
)

This happens even though the column limit is set to 120, so the invocation should fit into a single line just fine.

@BlankSpruce
Copy link
Owner

BlankSpruce commented Mar 24, 2024

This one is the funny one that perhaps I should revisit. In imaginary Python based system these two commands would be equivalent to:

set(
    name="CCACHE_SUPPORT", 
    value=["ON"], 
    cache="BOOL", 
    doc="Enable ccache support"
)

set(
    name="uuid_token_lengths",
    value=["8", "4", "4", "4", "12"],
    cache=None,
    doc=None
)

I've made this arbitrary choice initially that set with list of more than 4 arguments is transfomed to multi-line variant.

@BlankSpruce
Copy link
Owner

BlankSpruce commented Mar 25, 2024

I think I will close this one with an update to README and help. While favour-inlining tries its best to inline the code I think it should be done within reason hence the "favour", not the "force".
The threshold of 4 (arbitrary no doubt) for group of arguments without keyword (e.g. add_library, set, list(APPEND)) is a good enough heuristic for "this will explode into huge diff if we don't split into multiple lines early".

Examples where it applies:

set(FOO thing1 thing2 thing3 thing4)
set(FOO
    thing1
    thing2
    thing3
    thing4
    thing5
)
list(APPEND FOO thing1 thing2 thing3 thing4)
list(
    APPEND
    FOO
    thing1
    thing2
    thing3
    thing4
    thing5
)
add_library(FOO SHARED thing1 thing2 thing3 thing4)
add_library(
    FOO
    SHARED
    thing1
    thing2
    thing3
    thing4
    thing5
)

and where it doesn't:

target_link_libraries(FOO PUBLIC thing1 thing2 thing3 thing4)
target_link_libraries(
    FOO
    PUBLIC thing1 thing2 thing3 thing4 thing5
)

In hindsight I should have probably made favour-expansion (or some flavour of that) the default one but well, there are things we learn along the way.

What do you think?

@PatTheMav
Copy link
Author

PatTheMav commented Mar 25, 2024

Probably best to document this to make it explicit that inline favouring will not apply to set (and I'd guess also list(APPEND ...) with more than 4 elements.

It's fine to have these arbitrary internal rules, though if documented they can cut down on issue reports like this one.

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

No branches or pull requests

2 participants