clang-format example using Google style #679

bam241 · 2020-05-28T19:50:15Z

just a test on 2 files to see what would happened

bam241 · 2020-05-28T19:54:58Z

src/dagmc/DagMC.hpp

-   *     operation are fair game, the surface meshsets have triangles as members, but OBBs as children
-   *     but no querying is done, just assumptions that the tags exist.
+   * 1) The file is loaded and when we query the meshset, we find entities with
+   *the OBB_TREE tag 2) The OBBTreeTool assumes that any children of the entity


there is an issue here that would need to be fixed manually

ljacobson64 · 2020-05-28T20:15:42Z

I personally really like clang-format

bam241 · 2020-05-29T18:22:08Z

there it is.... this is a lot....

I used default Google style in clang-format which only sort #include by alphabetical order, does't do block.... we can configure include blocks if we'd like.
I also updated the housekeeping script to use clang-format instead of astyle....

bam241 · 2020-05-29T19:07:12Z

@gonuke @pshriwise @makeclean @ljacobson64 this is rdy, maybe not to be merged, but at least to discuss it :)

gonuke · 2020-07-06T18:57:05Z

I'm willing to make this change, but I'm concerned about the need for a manual fix. That's not practical given the way we use it.

bam241 · 2020-07-06T18:59:54Z

I am hoping that the manual fixes are just for the first time.

Those files that I fixed, are now passing clang-format without change, just clang-format seems to able to reformat them properly the first time.

ljacobson64 · 2020-07-06T19:14:25Z

I have also noticed that in very rare situations, you have to run clang-format twice in order to get the code to a state where it won't change again upon a further run of clang-format. This is not ideal, but again, very rare: out of ~89000 lines of C++ code in DAGMC, this only occurs in 3 lines. I don't think this is a big enough problem to worry about.

gonuke · 2020-07-06T19:33:05Z

It's only a problem if it happens every time that CI is launched. That is, even if its only a couple of lines, if it happens every time it will be a pain.

ljacobson64 · 2020-07-06T19:37:58Z

It should never be a problem in CI as long we run it twice the first time we run clang-format. From then on, it should only ever need to be run once.

bam241 · 2020-07-08T14:21:47Z

@gonuke, @ljacobson64 is correct,

in few cases clang-format is not good enough to properly fix the formatting, but one can fix it manually in a clang-format compliant way.

this is what I did here, we are passing the new clang-format test with some file I had to format manually.

if manual formatting does not work it is possible add flag to tell clang-format to ignore this part of the file. see src/dagmc/DagMCVersion.hpp.in

ljacobson64 · 2020-07-08T14:41:54Z

@bam241 I'm confused about what you mean by needing to format files manually

bam241 · 2020-07-08T14:50:01Z

so sometimes basically with running clang-format the end results is messy

see the example I mentioned on dagmc.hpp up here. I had to manually correct the formatting to get it a nice and a clang-format compliant form.

bam241 · 2021-01-20T19:32:11Z

To many commits have past, I'll redo this based on the actual develop state.

But we have to move quickly, as a lot is going to be changing a little....

bam241

The only manual change I had to make was in
src/overlap_check/test/overlap_check_test.cpp
src/overlap_check/test/overlap_check_test.hpp

bam241 · 2021-01-20T21:10:23Z

src/overlap_check/test/overlap_check_test.hpp

+#include <memory>
+
 #include "gtest/gtest.h"
+#include "moab/Core.hpp"



Reordering includes in the cpp files forced me to move some of the includes into the header file.

I think in any case it was bad practice to have some of the required header includes in the cpp file (and as they got loaded before the header, it was ok...)

bam241 · 2021-01-20T21:11:30Z

src/overlap_check/test/overlap_check_test.cpp

-#include <iostream>
+#include "overlap_check_test.hpp"
+
+#include <algorithm>
 #include <cmath>
 #include <cstdlib>
 #include <ctime>
-#include <vector>
+#include <iostream>
 #include <set>
-#include <algorithm>
-#include <memory>
-#include "moab/Core.hpp"
+#include <vector>

 #include "overlap.hpp"

-#include "overlap_check_test.hpp"
-
 void OverlapTest::SetUp() {


reordering includes broke compilation...

memory and moab/Core were included before overlap_check_test.hpp so compilation was ok, events they were needed by overlap_check_test.hpp and not included there...
I moved them in overlap_check_test.hpp....

bam241 · 2021-01-20T21:18:20Z

As so many file have to change, any PR merged before this, would force me to update this. So please review promptly :)

gonuke

My review is only based on the files that implement the change and not the style changes themselves. I assume we are comfortable with those style changes, and feel we can rely on future PRs to change/update individual aspects of style if we deem it necessary.

bam241 · 2021-01-20T23:39:44Z

There is few files that might need to be tweaked within the style

This assumes we are ok using google style from clang-format without any fancy option.

I don't necessary want to use google style, but I strongly believe we should pick a standard style and use it as is... It would make developers life so much easier...

pshriwise · 2021-01-21T01:20:48Z

I've become a fan of the C++ Core Guidelines. They're written by C++ language developers who have a deep understanding of the language and its roadmap. We don't currently enforce this as part of our PRs (yet), but we do have a .clang-format file in the repo which applies these guidelines.

helen-brooks · 2021-01-21T08:30:07Z

No problems from my end.

bam241 · 2021-01-21T12:27:35Z

I've become a fan of the C++ Core Guidelines. They're written by C++ language developers who have a deep understanding of the language and its roadmap. We don't currently enforce this as part of our PRs (yet), but we do have a .clang-format file in the repo which applies these guidelines.

what I am fearing with a so complicated and customized set of .clang-format file (from openMC) is that we make it more complicated for dev to use.

A simple .clang-format following a default style would be more practical and simpler to enforce...

gonuke · 2021-01-21T14:33:12Z

We should probably announce this changes since it may be a big task for anyone rebasing this into their branch

bam241 force-pushed the clang-format branch from 1ee108d to acdf6c5 Compare May 28, 2020 19:52

bam241 commented May 28, 2020

View reviewed changes

bam241 force-pushed the clang-format branch from acdf6c5 to 4fa3049 Compare May 29, 2020 18:19

bam241 added the PLEASE_REVIEW label May 29, 2020

bam241 changed the title ~~[TEST][DO NOT MERGE]clang-format example using Google style~~ [DO NOT MERGE]clang-format example using Google style May 29, 2020

bam241 force-pushed the clang-format branch from eed30da to 4968fc7 Compare January 20, 2021 19:37

install clang-format

e7f8eb7

bam241 force-pushed the clang-format branch from 3c95d10 to e7f8eb7 Compare January 20, 2021 19:46

bam241 added 10 commits January 20, 2021 13:47

adding news and .clang_format file

52cca05

applying clangformat to all the cpp files

c9abf9b

forgot 1

542192a

forgot antoher1

5abf9ab

forgot antoher1

121f238

forgot antoher1

2144096

forgot antoher1s

1ec52f9

forgot so much more

86bd81b

fixing typos...

d8eddbc

moving includes

5a1160f

bam241 added 2 commits January 20, 2021 14:53

fixing typo

eb7a5d6

moving includes

cf7cb94

bam241 changed the title ~~[DO NOT MERGE]clang-format example using Google style~~ clang-format example using Google style Jan 20, 2021

bam241 requested review from pshriwise, gonuke, shimwell, helen-brooks and ljacobson64 January 20, 2021 21:08

bam241 commented Jan 20, 2021

View reviewed changes

bam241 requested a review from makeclean January 20, 2021 21:18

gonuke approved these changes Jan 20, 2021

View reviewed changes

pshriwise merged commit d1431a8 into svalinn:develop Jan 21, 2021

bam241 deleted the clang-format branch November 28, 2024 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang-format example using Google style #679

clang-format example using Google style #679

bam241 commented May 28, 2020

bam241 May 28, 2020 •

edited

Loading

ljacobson64 commented May 28, 2020

bam241 commented May 29, 2020 •

edited

Loading

bam241 commented May 29, 2020

gonuke commented Jul 6, 2020

bam241 commented Jul 6, 2020

ljacobson64 commented Jul 6, 2020

gonuke commented Jul 6, 2020

ljacobson64 commented Jul 6, 2020

bam241 commented Jul 8, 2020

ljacobson64 commented Jul 8, 2020

bam241 commented Jul 8, 2020 •

edited

Loading

bam241 commented Jan 20, 2021

bam241 left a comment

bam241 Jan 20, 2021 •

edited

Loading

bam241 Jan 20, 2021 •

edited

Loading

bam241 commented Jan 20, 2021

gonuke left a comment

bam241 commented Jan 20, 2021 •

edited

Loading

pshriwise commented Jan 21, 2021

helen-brooks commented Jan 21, 2021

bam241 commented Jan 21, 2021

gonuke commented Jan 21, 2021

clang-format example using Google style #679

clang-format example using Google style #679

Conversation

bam241 commented May 28, 2020

bam241 May 28, 2020 • edited Loading

Choose a reason for hiding this comment

ljacobson64 commented May 28, 2020

bam241 commented May 29, 2020 • edited Loading

bam241 commented May 29, 2020

gonuke commented Jul 6, 2020

bam241 commented Jul 6, 2020

ljacobson64 commented Jul 6, 2020

gonuke commented Jul 6, 2020

ljacobson64 commented Jul 6, 2020

bam241 commented Jul 8, 2020

ljacobson64 commented Jul 8, 2020

bam241 commented Jul 8, 2020 • edited Loading

bam241 commented Jan 20, 2021

bam241 left a comment

Choose a reason for hiding this comment

bam241 Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

bam241 Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

bam241 commented Jan 20, 2021

gonuke left a comment

Choose a reason for hiding this comment

bam241 commented Jan 20, 2021 • edited Loading

pshriwise commented Jan 21, 2021

helen-brooks commented Jan 21, 2021

bam241 commented Jan 21, 2021

gonuke commented Jan 21, 2021

bam241 May 28, 2020 •

edited

Loading

bam241 commented May 29, 2020 •

edited

Loading

bam241 commented Jul 8, 2020 •

edited

Loading

bam241 Jan 20, 2021 •

edited

Loading

bam241 Jan 20, 2021 •

edited

Loading

bam241 commented Jan 20, 2021 •

edited

Loading