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

Competing heuristics for 2+ argument statements that are one character short of fitting on a single line. #51

Closed
mikew-lunarg opened this issue Jul 5, 2018 · 10 comments
Labels
acknowledged I've seen the report. I may not have anything more to say just yet! bug

Comments

@mikew-lunarg
Copy link

  1. # cmake-format: off gets indented, the matching # cmake-format: on remains unindented. Personally I prefer them both unindented as before.
  2. This set() command is formatted very differently than other set() commands before and after. Not clear why.

Thanks very much!

@cheshirekow
Copy link
Owner

cheshirekow commented Jul 7, 2018

Hm... on second thought, (1) is somewhat intentional. While it may be possible to leave both unformatted it's a little confusing to think where in the stream of tokens we start/stop formatting. The way it is implemented now is "From the end of the # cmake-format: off token to the start of # cmake-format: on token, which is very straight forward to identify. My guess is that what you are requesting is something like: "From the end of the last NEWLINE token before the occurance of # cmake-format:off or the start of the file if there is none", which would preclude putting # cmake-format: off at the end of a statement. Not a terrible compromise, but I'd have to think about that further if you'd like to make that a feature request.

As for (2): I get this with python 2.7.12 and 3.5.2:

set(cubepp_SRCS
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/main.mm
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/AppDelegate.mm
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/DemoViewController.mm)

set(cubepp_HDRS ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/AppDelegate.h
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/DemoViewController.h)

set(cubepp_RESOURCES ${CMAKE_BINARY_DIR}/staging-json/MoltenVK_icd.json
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/Resources/LunarGIcon.icns)

@mikew-lunarg
Copy link
Author

About (1): I can see your point. However

# cmake-format: really isn't just a typical comment to be flowed with the rest. it's an active command to the tool, more like a C pre-processor directive. The broken symmetry between # cmake-format: on and # cmake-format: off is rather jarring.

Re (2): I ran that formatting on Ubuntu LTS 16.04 with the following. I can retry on other system configs in a couple days, see if the issue travels.

$ python --version
Python 2.7.12
$ python3 --version
Python 3.5.2
$ cmake --version
cmake version 3.5.1

@cheshirekow
Copy link
Owner

Yeah, I've got the same setup with ubuntu and python, and still get that formatting. I tried that with stdout as output and with -i and got it both times. My guess is maybe if you were running v0.4.0rc1 through the environment PYTHONPATH maybe the environment was cleared for that file or something?

Can you paste the command you used? I'll see if I can replicate the behavior.

@mikew-lunarg
Copy link
Author

Ok, I've tried it on Ubuntu and Windows, 0.4.0rc1 and 0.4.0, pip-installed and tarball.

I consistently get the same weird formatting on that set() command, on every tested platform with 0.4.0.

My 0.4.0* results are all identical, see 1 2 3

I appreciate you're not able to repro mine, but I'm not able to repro yours!

My 0.4.0* tests all begin with the 0.3.6 baseline, as shown in my git history. Here are the invocation scripts I used in each test. (The 0.3.6 is slightly different, to workaround the comment bug)...

These 0.4.0* are all basically identical...

@mikew-lunarg
Copy link
Author

Trying another platform, I also get the same consistently weird formatting w 0.4.0 pip on Raspbian.
(on a Pi Zero. Had to try it ;-)

@cheshirekow
Copy link
Owner

The difference in behavior is apparently due to your .cmake-format.py. If I use your config file I'm able to reproduce (previously tested the file in isolation with default config). Now that I can reproduce, I can try to figure out whats going on.

@cheshirekow
Copy link
Owner

Ok, so note that cubepp_HDRS ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/AppDelegate.h ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/DemoViewController.h is exactly 132 characters wide (your configured column limit). So that is what leads to the slight weirdness of the dangling paren, however the formatting:

set(cubepp_HDRS 
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/AppDelegate.h
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/DemoViewController.h)

Should be the preferred formatting. I'm not sure why it's passing over that one. I'll update when I figure out what's going on.

@cheshirekow
Copy link
Owner

Ok, I see the problem. It has to do with the fact that everything "almost" fits on one line. It is just one character short. There are two heuristics that come into play in this situation and they seem to conflict.

Note to self for how to fix:

  1. HPACking single arguments doesn't account for the extra character required for the RPAREN
  2. RPAREN is not dangled in VPACK for a statement. The idea was that if it would overflow we should go to NVPACK next instead of dangling.

@cheshirekow
Copy link
Owner

cheshirekow commented Jul 14, 2018

As a temporary workaround, I suggest adding a blank comment at the end:

set(cubepp_HDRS
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/AppDelegate.h #
    ${CMAKE_CURRENT_SOURCE_DIR}/macOS/cubepp/DemoViewController.h)

@cheshirekow cheshirekow changed the title Minor weirdnesses in 0.4.0rc1 Competing heuristics for 2+ argument statements that are one character short of fitting on a single line. Jul 14, 2018
@cheshirekow cheshirekow added bug acknowledged I've seen the report. I may not have anything more to say just yet! labels Jul 14, 2018
@mikew-lunarg
Copy link
Author

Cool! Thanks very much for the detective work.

cheshirekow added a commit that referenced this issue Sep 16, 2018
* Add algorithm order config option
* Add user specified fence regex config option
* Add user specified ruler regex config option
* Add config option to disable comment formatting altogether
* Fix get_config bug in ``__main__``
* Fix missing elseif command specification
* Fix missing elseif/else paren spacing when specified
* Add enable_markup config option
* Fix kwargstack early breaking in conditionals
* Add some notes for developers.
* Add warning if formatter is inactive at the end of a print
* Add config options to preserve first comment or any matching a regex

Closed issues:

* Fixes #34 - if conditions with many elements
* Closes #35 - break_before_args
* Resolves #42 - user specified string for fencing
* Resolves #43 - allow custom string for rulers
* Fixes #45 - config file not loaded properly
* Fixes #51 - competing herustics for 2+ argument statements
* Resolves #60 - option to not reflow initial comment block
* Resolves #61 - add non-builtin commands
* Fixes #63 - elseif like if
* Resolves #65 - warn if off doesn't have corresponding on
* Closes #67 - global option to not format comments
* Fixes #68 - seperate-ctrl-name-with-space
cheshirekow added a commit that referenced this issue Sep 17, 2018
* Add visual studio code extension
* Add algorithm order config option
* Add user specified fence regex config option
* Add user specified ruler regex config option
* Add config option to disable comment formatting altogether
* Fix get_config bug in ``__main__``
* Fix missing elseif command specification
* Fix missing elseif/else paren spacing when specified
* Add enable_markup config option
* Fix kwargstack early breaking in conditionals
* Add some notes for developers.
* Add warning if formatter is inactive at the end of a print
* Add config options to preserve first comment or any matching a regex

Closed issues:

* Fixes #34 - if conditions with many elements
* Closes #35 - break_before_args
* Resolves #42 - user specified string for fencing
* Resolves #43 - allow custom string for rulers
* Fixes #45 - config file not loaded properly
* Fixes #51 - competing herustics for 2+ argument statements
* Resolves #60 - option to not reflow initial comment block
* Resolves #61 - add non-builtin commands
* Fixes #63 - elseif like if
* Resolves #65 - warn if off doesn't have corresponding on
* Closes #67 - global option to not format comments
* Fixes #68 - seperate-ctrl-name-with-space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged I've seen the report. I may not have anything more to say just yet! bug
Projects
None yet
Development

No branches or pull requests

2 participants