I was struggling with myself because I don't want to derail the current progress,
however, I figured it is still worth pointing this out (again), the earlier the better.
Hi Johannes,
I believe every file committed to flang runs through clang-format. There are three formats being used. The front-end files use the llvm style with some modifications. The files related to MLIR follow the MLIR style. And the files closest to LLVM follow the LLVM style. This all seems reasonable to me.
There was a huge file-renaming pass done over the code before the initial merge. Do you have an example that doesn't follow the convention?
The compilation slowness may be related to heavy use of std::variant. I'm not trying to be difficult here but perhaps you can bring up the compilation speed issue with cfe? Michael Kruse measured the time required to compile flang with gcc, vc++, and clang; my recollection is that clang was slowest of the batch.
- Steve
Hi Johannes,
I believe every file committed to flang runs through clang-format. There are three formats being used. The front-end files use the llvm style with some modifications. The files related to MLIR follow the MLIR style. And the files closest to LLVM follow the LLVM style. This all seems reasonable to me.
The webpage say "clang format of llvm 7" which is not what phabricator runs and not what we should announce as style guide.
Three formats doesn't strike me as reasonable. What is the benefit over two, or one, either would make reviews and code writing easier.
To play devils advocate, why stop at three, every folder could have a distinct style, where do we draw the line?
Note that this is not only about clang format here but also naming and other things (some of which clang-tidy would flag).
There was a huge file-renaming pass done over the code before the initial merge. Do you have an example that doesn't follow the convention?
I just run:
for folder in {llvm,mlir,clang,flang}; do echo -n "Folder $folder, lower case:"; find $folder/lib -name '[a-z]*.cpp' | wc -l; echo -n " upper case: "; find $folder/lib -name '[A-Z]*.cpp' | wc -l; done
Folder llvm, lower case:6
upper case: 2226
Folder mlir, lower case:0
upper case: 381
Folder clang, lower case:0
upper case: 758
Folder flang, lower case:97
upper case: 36
So llvm, mlir, and clang have basically no files starting with a lower case letter, flang has a 3:1 ratio of them.
Note that this is only checking /lib, other places need to be checked too.
Similarly, the advocated use of `-` (over `_`) neither is really used outside of flang:
Folder llvm, # of '_' uses:7
# of '-' uses: 0
Folder mlir, # of '_' uses:0
# of '-' uses: 0
Folder clang, # of '_' uses:0
# of '-' uses: 4
Folder flang, # of '_' uses:0
# of '-' uses: 64
So these are obvious differences when it comes to naming conventions, I did not spend too much time on it though.
The compilation slowness may be related to heavy use of std::variant. I'm not trying to be difficult here but perhaps you can bring up the compilation speed issue with cfe? Michael Kruse measured the time required to compile flang with gcc, vc++, and clang; my recollection is that clang was slowest of the batch.
We can "blame" clang for being slow and even try to improve it, that doesn't change the fact that the "suggestions" online are in direct
contradiction to common practices across the project. It happens that these "suggestion" are also a likely candidate for the build time
issues and a reasonable reaction would be to backtrack and follow the path plotted by the rest of LLVM to avoid them.
~ Johannes
I’m sure the reference to LLVM 7 is out of date. I’m pretty sure that the devs keep up with what’s in phabricator. I’ll let other comment.
Why three?