LLVM coding conventions an the OpenMP runtime

Hello,

We are considering the possibility of doing a conversion of the OpenMP runtime code to better comply with the LLVM coding conventions in the mid- to late-September time frame. This would most likely involve running the code through clang-format with the LLVM style option, as well as correcting any other glaring violations of the coding conventions.

It would probably not involve renaming anything to adhere to naming conventions.

However, we’ve noted that LLVM’s coding standards document says the following:

“There are some conventions that are not uniformly followed in the code base (e.g. the naming convention). This is because they are relatively new, and a lot of code was written before they were put in place. Our long term goal is for the entire codebase to follow the convention, but we explicitly do not want patches that do large-scale reformating of existing code. On the other hand, it is reasonable to rename the methods of a class if you’re about to change it in some other way. Just do the reformating as a separate commit from the functionality change.“

This would definitely be a large-scale reformatting.

So I just wanted to get some feedback on this before we make plans to do this.

Thanks!

Terry

Hi Terry,

IMO we should for now stay with the current coding standard as it is currently consistently used within the runtime (4 spaces indention, naming etc.).

That said, there was a proposal of moving the OpenMP runtime into parallel_libs (which I completely support btw).

If the whole code is then recommitted anyway, I think it is safe to do the cleanups in that process.

Regards,

Jonas

Can we not compound two distinct and unrelated issues. Proper code
formatting impacts everything now and there's no blocker on it needing
to be moved.

I'm strongly in favor of going towards a consistent style which is
similar to llvm/clang. However, if others feel strongly that it's
disruptive I think we should be sensitive to their views. I realize
that Intel is maintaining two trees already and I wouldn't want to
make their job any harder, just for the sake of cosmetic candy.

Chris,

I didn't: I'm currently in favor of NOT doing the conversion as also said in the coding standards. Full stop.

However I just wanted to express that I'm not against (more precisely: strongly support) to do the reformatting when moving the code base anyway.
I agree that this is a different matter and has to be discussed separately but it may be a compromise on this discussion.

Thanks,
Jonas

I can understand why Intel would be strongly against a larger format,
can you give some data points for your use case? Besides just a
"feeling" what's the rationale for strongly against

Thanks

As already stated I think that we currently have a consistent coding style inside the runtime.
I agree that aligning to LLVM/Clang should be the long-term goal but IMO "cosmetic candy" doesn't warrant a full reformat in place. (I'm supported here by the LLVM coding standard itself!)

To put up more practical reasons: There is quite some research and experimental implementation based on this repository.
It will be hard as hell to update and work with them when we touch every single line with whitespace throughout the code.

Regards,
Jonas

Pragmatically and just my view - If the research is open and there's a
plan to integrate it back I'm empathetic. If it's closed and just an
outside fork, I don't care what you do and it shouldn't block an open
source project. Long term plans start somewhere - so even if this
clean-up doesn't happen now, I think it makes sense to figure out who
will impact and break it into small steps.

Again just my view, but I strongly dislike some of the coding
practices being employed. I'd love to see some things which I consider
"fugly hacks" removed.

In terms of the actual logistics - if the entire clean-up is done as a
single commit - it should be fairly easy for that to rebase against an
out-of-tree fork. Worst case the research just isn't in sync with
upstream, but not all research projects track git HEAD or svn ToT..

From: C Bergström [mailto:cbergstrom@pathscale.com]
Sent: Tuesday, August 09, 2016 10:23 AM
To: Hahnfeld, Jonas
Cc: Wilmarth, Terry L; LLVM-OpenMP (openmp-dev@lists.llvm.org)
Subject: Re: [Openmp-dev] LLVM coding conventions an the OpenMP
runtime

Pragmatically and just my view - If the research is open and there's a plan to
integrate it back I'm empathetic. If it's closed and just an outside fork, I don't
care what you do and it shouldn't block an open source project. Long term
plans start somewhere - so even if this clean-up doesn't happen now, I think
it makes sense to figure out who will impact and break it into small steps.

Some research ends up being standardized and is given back, but I don't want to discuss this here.
This may be a weak argument but I think it's valuable to have it in mind...

Again just my view, but I strongly dislike some of the coding practices being
employed. I'd love to see some things which I consider "fugly hacks"
removed.

If that's anything else than "cosmetic candy" I think nobody will speak up against patches :wink:

In terms of the actual logistics - if the entire clean-up is done as a single
commit - it should be fairly easy for that to rebase against an out-of-tree
fork. Worst case the research just isn't in sync with upstream, but not all
research projects track git HEAD or svn ToT..

Disagree here, what I've seen so far: Neither git nor SVN can easily handle whitespace reformatting on rebase or merge.

Cheers,
Jonas

In terms of the actual logistics - if the entire clean-up is done as a single
commit - it should be fairly easy for that to rebase against an out-of-tree
fork. Worst case the research just isn't in sync with upstream, but not all
research projects track git HEAD or svn ToT..

Disagree here, what I've seen so far: Neither git nor SVN can easily handle whitespace reformatting on rebase or merge.

I'm not sure what you're exactly disagreeing with, but to clarify - I
mean taking the master branch commit, exporting it as a patch and
using patch[0] to fuzzy apply it. This is not a zero cost effort, but
it's a 1x cost and hopefully wouldn't be so expensive. I think it
would largely depend on how localized your changes are. I suspect it's
unlikely that you've changed every file and most changes which would
conflict are likely 1-4 files with probably 1-2 being more LOC impact.
This is just the cost of an out-of-tree fork you must maintain. clang
and llvm afaik also aren't super friendly to out-of-tree branches. For
example, I strongly objected to the old r600/ AMD GPU target being
renamed "amdgpu". The name change did nothing technical, but it did
make it a pita for us.. I lost the argument and we just had to pay the
price to stay in sync.. c'est la vie..

Anyway - if it's not open source research, then my view is it carries
little to no weight. Intel is the exception since catering to them
will hopefully make things easier long term, as I hope they continue
to migrate over bug fixes and stuff.

[0] patch Options (Comparing and Merging Files)

Hi Terry,

I’m thrilled to hear that you’re interested in doing this. FYI, there is an exactly analogous discussion going on in the LLDB space, where they are similarly reformatting the world to make it more consistent with LLVM standards.

I think that doing a large scale reformating like this is a great thing to do since it fixes a large problem. My advice is to do it all as one major commit. This way there is one discontinuity in the version control stream, and not lots of little ones everywhere. This is the approach the LLDB community is preferring to take, and I think it is the best approach that gets to the right answer.

-Chris

+1, this is consistent with the overall LLVM approach.

The open source project can’t be concerned with every private fork that may be maintained, and worrying about that often cuts against the bests interests of the project (which is to always move forward).

Additionally, it is arguably *better* for the open source project to have a bit of churn in the code, because this makes it more painful to maintain out of tree branches. This is one way to encourage these private branches to be contributed back to mainline, instead of being kept proprietary.

-Chris

I strongly disagree open source should ever use tactics to encourage private forks to integrate with upstream.. I'd always go with the carrot..

The flow which is ideal and won't carry hard feelings
Use > Love > Contribute

Original Message

A second thing we are considering (and apologies for leaving this out of the original message) is the renaming of .c files to .cpp. As the runtime evolves, C++ has been used more and more.

So the file renaming is something else to consider, either separately or in conjunction with the code formatting changes.

-Terry

From: "Terry L via Openmp-dev Wilmarth" <openmp-dev@lists.llvm.org>
To: "Chris Lattner" <clattner@apple.com>, openmp-dev@lists.llvm.org
Sent: Thursday, August 11, 2016 1:06:09 PM
Subject: Re: [Openmp-dev] LLVM coding conventions an the OpenMP runtime

A second thing we are considering (and apologies for leaving this out
of the original message) is the renaming of .c files to .cpp. As
the runtime evolves, C++ has been used more and more.

Yes, please. That would be great.

So the file renaming is something else to consider, either separately
or in conjunction with the code formatting changes.

Please do this in a separate commit. On the off-chance that this introduces a behavioral change, I'd like to be able to bisect to just this change.

-Hal