Pcl: Create a CI job that checks formatting

[taketwo]

• [x] Compose a shell script/command that formats the code and outputs diff
• [x] Create a minimal Docker image that can run the command (here)
• [x] Setup Azure pipeline
• [x] Format the whitelisted modules such that the new pipeline passes on master
  
  
  
  • [x] 2d #3343
  
  
  • [ ] ~cuda~
  
  
  • [ ] ~gpu~
  
  
  • [x] ml #3363, #3393, #3416
  
  
  • [x] simulation #3356
  
  
  • [x] stereo #3344
  
  
  • [ ] ~tracking~
  
  

[taketwo]

I've added a new Azure pipeline (currently in my fork). Example run of the pipeline, the diff is printed in one of the steps and is also published as an artifact.

In the process I have bumped into some strange behavior of clang-format. As you can see in the commit history of my branch, after creating the pipeline I pushed a commit that formatted everything (using our format.sh script). However, the pipeline failed, producing a small diff. In the beginning I thought this is due to a difference in clang-format versions on my local machine and CI (8 vs. 6). However, when I tried to run format again locally, it also produced a small diff! So I committed and pushed, yet CI still wasn't happy, though with even smaller diff this time. This last bit is indeed due to differences in clang-format versions, I have confirmed it locally.

So now the question is what to do with our minimum clang-format version requirement.

[kunaltyagi]

This last bit is indeed due to differences in clang-format versions

Who is using what versions in this case?

when I tried to run format again locally, it also produced a small diff

So running clang-format twice gave different results?

what to do with our minimum clang-format version

Either fix it or find the difference and correct for them explicitly via modifications in the .clang-format file

[taketwo]

Who is using what versions in this case?

The CI image has version 6, and locally I have version 8. There is no particular reason for this though, CI can be updated to have version 8 or any other version for that matter.

So running clang-format twice gave different results?

Running clang-format-8 two times sequentially gives different results from running it a single time. You can reproduce this on current master by running ./.dev/format.shwhich clang-format-8., staging changes, and then running it again. You will notice that three files are modified in the second run.

find the difference and correct for them explicitly via modifications in the .clang-format file

Would you mind to have a look at this?

[kunaltyagi]

three files are modified in the second run

Ah yes. I remember this weird behavior now. If the changes are too drastic or dependent on some other flag, then it can take multiple clang-formats to achieve equilibrium. This usually happens when clag-format gets confused with some tokens since it's not really a full-blown compiler. I don't know of any solution except running it in a loop. This would need to be done only when new modules are added under the format command

touch old.diff
while
    ./.dev/format.sh # the actual format command
    git diff > new.diff
    diff old.diff new.diff > delta.diff
    (( [ -s delta.diff ] ))
do
    mv new.diff old.diff
done

The rest of pipeline should stay the same.

Regarding the difference between version 6 and 8, let me check. It might not be possible to find a set of flags to maintain invariance between them, in which case we can drop support for older versions because in the last discussion, the version didn't matter much.

[taketwo]

So when we apply v6 formatting to the code that was v8 formatted (after reaching equilibrium), we get the following diff:

--- a/cuda/features/include/pcl/cuda/features/normal_3d.h
+++ b/cuda/features/include/pcl/cuda/features/normal_3d.h
@@ -48,8 +48,7 @@ namespace cuda {
 // bias!! contact Nico for details :P
 template <typename InputIteratorT,
           typename OutputIteratorT,
-          template <typename>
-          class Storage>
+          template <typename> class Storage>
 void
 computePointNormals(InputIteratorT begin,
                     InputIteratorT end,

It looks like this has to do with the rule AlwaysBreakTemplateDeclarations: true. Personally, I wouldn't insert a break here, but if this is the "modern" behavior, then we probably better stick with it.

[kunaltyagi]

I was not able to reproduce this on the online configurator

Maybe this is a platform issue? Let me try to reproduce this on my computer later next Monday

[kunaltyagi]

the rule AlwaysBreakTemplateDeclarations: true

This along with BinPackParameters: false cause the behavior. clang-format-6.0 doesn't seem to treat the template parameters as parameters. All future versions however have a consistent behavior.

I also noticed another diff between clang-format-9 and the rest. v9 is treating this as a parameter declaration while v8 and before treat this as a function declaration. The missing parameter name and the syntax involve the most-vexing parse of c++ but by the rules it should pick the function option, not the variable alternative. This diff goes away if I give a name to the parameter. This shows that clang-format isn't perfect.

--- a/gpu/kinfu_large_scale/tools/record_maps_rgb.cpp
+++ b/gpu/kinfu_large_scale/tools/record_maps_rgb.cpp
@@ -87,8 +87,8 @@ public:
   MapsBuffer() {}

   bool
-  pushBack(boost::shared_ptr<const MapsRgb>); // thread-save wrapper for push_back()
-                                              // method of ciruclar_buffer
+      pushBack(boost::shared_ptr<const MapsRgb>); // thread-save wrapper for push_back()
+                                                  // method of ciruclar_buffer

   boost::shared_ptr<const MapsRgb>
   getFront(bool); // thread-save wrapper for front() method of ciruclar_buffer

I propose that we put the minimum required clang-format to 7 (to keep compatibility with future) and resolve the tiny diffs that might occur manually like this one line diff in the entire PCL repo

[taketwo]

Thanks for digging into this, I agree with raising requirement to 7. And on CI we should rather avoid 9 and use 8, right?

[kunaltyagi]

we should rather avoid 9 and use 8, right?

Yes. 7 and 8 are perfect copies without any effort. 9 is ok IFF we give a variable name in the problematic function above.

[taketwo]

OK. So to finish this off we need to apply formatting to whitelisted modules. I've submitted #3343, more to follow. It would be great if you could skim them through and leave a GitHub review. There is not much going on inside, but still better to have another pair of eyes on them for a sanity check.

[taketwo]

At the moment, four white-listed modules are formatted. It will take an unknown amount of time until the rest follow suit. Thus I was thinking about reducing the whitelist to these four modules and finally enabling formatting check on CI. In the future, once another module is formatted, it can be added to the whitelist.