cheshirekow / cmake_format

Source code formatter for cmake listfiles.
GNU General Public License v3.0
954 stars 105 forks source link

Feature Request: format PRIVATE/PUBLIC/INTERFACE entries with constant indentation #206

Open reddwarf69 opened 4 years ago

reddwarf69 commented 4 years ago

I have been formatting my PRIVATE/PUBLIC/INTERFACE commands like this

target_link_libraries(
    targetA
    INTERFACE
        targetB
        targetC
    PUBLIC
        targetD
        targetE
)

I would not mind if it were

target_link_libraries(targetA
    INTERFACE
        targetB
        targetC
    PUBLIC
        targetD
        targetE
)

But I really like: a) Always a single dependency by line b) The constant indentation level of targetB and targetD, even if they are under specifiers (INTERFACE and PUBLIC) with different lengths.

After playing with the configuration file I have not seen any option to allow this. Would it be possible to make cmake-format format like this?

cheshirekow commented 4 years ago
target_link_libraries(targetA
                    # ^^^ unwrapped first argument

Is feature request #137 (in progress)

a) Always a single dependency by line

cmake-format currently does not have any knobs that distinguish between the positional arguments in this case versus any other case. The rules for wrapping positional arguments are the same. All it knows is that INTERFACE takes a sequence of positional arguments. These arguments are indistinguishable from the positional arguments of, for example, the set() command. So if you change the rules for one case you change them for both. It's tough to achieve what you want without adversely affecting the format of other commands.

In the future there will be an option to tune how a "homogeneous list" is formatted, as different to an untagged sequence of arguments. For now, you can play around with max_pargs_hwrap.

b) The constant indentation level of targetB and targetD

Hm... I'm a bit confused. If I understand correctly, what you're requesting here is already the case. If the arguments of INTERFACE and PUBLIC are both wrapped, then they will be aligned in the same column. Can you restate what you mean here?

You may want to look at the documentation for the formatting algorithm to better understand what can be done. If you want keyword groups to be nested before they are wrapped then you can accomplish that with the following configuration:

with section("format"):

  layout_passes = {
    "KwargGroupNode": [(0, True),  (5, True)]
  }
reddwarf69 commented 4 years ago

b) The constant indentation level of targetB and targetD

Hm... I'm a bit confused. If I understand correctly, what you're requesting here is already the case. If the arguments of INTERFACE and PUBLIC are both wrapped, then they will be aligned in the same column. Can you restate what you mean here?

I was just trying to avoid any confusion between

target_link_libraries(
    targetA
    INTERFACE
        targetB
        targetC
    PUBLIC
        targetD
        targetE
)

and

target_link_libraries(
    targetA
    INTERFACE targetB
              targetC
    PUBLIC targetD
           targetE
)
cheshirekow commented 4 years ago

Ok, so try the config I mentioned above and let me know if that provides what you're looking for. In the language of the formatting algorithm, you want the keyword argument subtrees to "nest" before they "wrap".

cheshirekow commented 4 years ago

ping. Does this configuration do what you're looking for?

comkieffer commented 4 years ago

Not OP, but I was looking for the same thing and this fixed it for me.

comkieffer commented 4 years ago

Whelp. I was a bit too fast to answer there.

That works most of the time but occasionally produces some weird output:

install(DIRECTORY
          sorurce_dir/ DESTINATION
                    dest_dir)

It seems like DESTINATION should be on a new line. I can work around it by setting max_subgroups_hwrap = 1 but that feels like overkill.

cheshirekow commented 4 years ago

@comkieffer It is illustrative to inspect the format tree for your example:

└─ BodyNode(BODY),(passno=0,wrap=F,ok=T) pos:(0,0) ext:(0,34)
    └─ StatementNode(STATEMENT),(passno=0,wrap=F,ok=T) pos:(0,0) ext:(0,34)
        ├─ ScalarNode(FUNNAME),(passno=0,wrap=F,ok=T) pos:(0,0) ext:(0,7)
        ├─ ParenNode(LPAREN),(passno=0,wrap=F,ok=T) pos:(0,7) ext:(0,8)
        ├─ ArgGroupNode(ARGGROUP),(passno=0,wrap=F,ok=T) pos:(0,8) ext:(0,34)            <--- Note (1)
        │   ├─ KwargGroupNode(KWARGGROUP),(passno=0,wrap=T,ok=T) pos:(0,8) ext:(0,22)
        │   │   ├─ ScalarNode(KEYWORD),(passno=0,wrap=F,ok=T) pos:(0,8) ext:(0,17)
        │   │   └─ PargGroupNode(PARGGROUP),(passno=0,wrap=F,ok=T) pos:(1,10) ext:(1,22)
        │   │       └─ ScalarNode(ARGUMENT),(passno=0,wrap=F,ok=T) pos:(1,10) ext:(0,22)
        │   └─ KwargGroupNode(KWARGGROUP),(passno=0,wrap=T,ok=T) pos:(1,23) ext:(0,34)   <---- Note (2)
        │       ├─ ScalarNode(KEYWORD),(passno=0,wrap=F,ok=T) pos:(1,23) ext:(0,34)
        │       └─ PargGroupNode(PARGGROUP),(passno=0,wrap=F,ok=T) pos:(2,25) ext:(1,33)
        │           └─ ScalarNode(ARGUMENT),(passno=0,wrap=F,ok=T) pos:(2,25) ext:(0,33)
        └─ ParenNode(RPAREN),(passno=0,wrap=F,ok=T) pos:(2,33) ext:(0,34)

The default behavior for attempts are

0. single line
1. statement wraps (child is nested)
2. argument groups wrap (parent of positional or keyword groups) 
3. positional arguments wrap
4. keyword groups nest

So I think nested keyword arguments at pass zero is too early and causing the weird case you're seeing. I guess you just want the keyword arguments to nest before their positional arguments to maybe try moving it to pass number 4 instead of 0:

with section("format"):

  layout_passes = {
    "KwargGroupNode": [(0, False), (4, True),  (5, True)]
  }

Alternatively, instead of making keyword arguments nest earlier, you could try to make positional arguments wrap later:

with section("format"):

  layout_passes = {
    "PargGroupNode": [(0, False), (5, True)]
  }
cheshirekow commented 4 years ago

Ping again. @comkieffer @RedDwarf69 anything else to say here?

comkieffer commented 4 years ago

I tried the two configuration samples you provided above, but they don't provide the desired result:

target_link_libraries(
  targetA
  INTERFACE targetB targetC
  PUBLIC targetD targetE)

install(DIRECTORY sorurce_dir/
        DESTINATION dest_dir)

However, I noticed that the formatting bug on the install command, when using the first configuration you suggested, goes away when the line is long enough (I set line_width=30 in the configuration). :

install(
  DIRECTORY
    source_dir/
    another_source/
    library_dir/
  DESTINATION
    dest_dir)
install(
  DIRECTORY
    source_dir/ DESTINATION
                  dest_dir)