Apply clang-format to simulation module #3356
Conversation
|
I don't think by default clang-format will automatically make something like into unless specified in the config file, please correct me if I am wrong. IMO this makes the code less readable and it creates more lines for no reason. I am curious to know what's the main reason behind this decision. |
| 0.00000008f, 0.00000007f, 0.00000006f, 0.00000006f, 0.00000005f, | ||
| 0.00000004f, 0.00000004f, 0.00000003f, 0.00000003f, 0.00000003f, | ||
| 0.00000002f}; | ||
| float normal_sigma0x5_normal1x0_range0to3_step0x01[] = { |
There was a problem hiding this comment.
Maybe update to use constexpr function returning to a std::array. Too many magic numbers. make them go away
| { | ||
| if (a%2 || b%2) return level; | ||
| while (true) { | ||
| if (a % 2 || b % 2) |
There was a problem hiding this comment.
Plus the function (by documentation) smells fishy. For example: EXPECT_TRUE(max_level(1025, 4095), 10) fails.
So many questions...
Is it a cleanly divisible test? Are a and b non-negative? Are they powers of 2?
This function is only used twice (L163, L865) so it might be depending on special behavior since there's no test for this
| write_file_ = false; | ||
| } | ||
| } | ||
|
|
||
| // Handle normal keys | ||
| void | ||
| on_keyboard (unsigned char key, int, int) | ||
| on_keyboard(unsigned char key, int, int) | ||
| { | ||
| double speed = 0.1; | ||
|
|
||
| if (key == 27) |
There was a problem hiding this comment.
Will switch-case make better sense than if-else ladder? Or even a std::tolower to reduce the code density
There was a problem hiding this comment.
Sure. But I wouldn't mix formatting and refactoring. A follow-up PR is welcome ;)
|
@jasjuang You are right, we have this rule explicitly in the config file.
I have the opposite opinion. In a template-heavy library like PCL, return types are often long. The method names, when prefixed with class and namespace names (as it usually happens in HPP files), are also long. Therefore, I believe it's easier to visually parse function declarations when the type is put on a separate line.
Here I also see a benefit since it consistently creates more lines. As explained above, return type together with function name are often long and in many cases exceed line length, so a line break is required anyway. With this rule, we have a line break always, so the look of the code is more homogeneous. I would say it's not like either option is better than another; they both have pros and cons. Therefore any choice would have been opinionated. So we just kept the old PCL style guide, avoiding making choices ;) @kunaltyagi Thanks for the review! I will go through it later. |
| { | ||
| if (a%2 || b%2) return level; | ||
| while (true) { | ||
| if (a % 2 || b % 2) |
| write_file_ = false; | ||
| } | ||
| } | ||
|
|
||
| // Handle normal keys | ||
| void | ||
| on_keyboard (unsigned char key, int, int) | ||
| on_keyboard(unsigned char key, int, int) | ||
| { | ||
| double speed = 0.1; | ||
|
|
||
| if (key == 27) |
There was a problem hiding this comment.
Sure. But I wouldn't mix formatting and refactoring. A follow-up PR is welcome ;)
| print_info ("\n"); | ||
|
|
||
| print_info ("\n(Note: for multiple .pcd files, provide multiple -{fc,ps,opaque} parameters; they will be automatically assigned to the right file)\n"); | ||
| print_error("Syntax is: %s <file_name 1..N>.<pcd or vtk> <options>\n", argv[0]); |
There was a problem hiding this comment.
I'm not so sure what to do about such help printing functions (we will see more of them in other modules). The formatting is messed up now, but is disabling clang-format completely a good idea?
There was a problem hiding this comment.
They way they are written, any further formatting isn't expected. The alternative would be using fixed width columns for output (std::setw or "%{amount}s", with amount in negative for left-align) which will align them normally without having to rely on the code layout.
But this breaks the visual aspect of the code.
There was a problem hiding this comment.
Yeah, and then this mixing of info/value printing to colorize the output... I'm leaning towards leaving this as-is for now.
taketwo
force-pushed
the
format-simulation
branch
from
b995ce6
to
be8a7b7
Compare
No description provided.