search

E3SM-Project / HOMMEXX

Clone of ACME for CMDV-SE project to convert HOMME to C++
11 stars 0 forks source link

Running format on cxx folder #347

Closed oksanaguba closed 5 years ago

oksanaguba commented 6 years ago

Good enough? Can i push @ambrad @mfdeakin-sandia ?

mfdeakin-sandia commented 6 years ago

I'd like to merge cam_fixes first, sorry

oksanaguba commented 6 years ago

fine with me -- i will redo after your merge. i only did this today cause i promised.

ambrad commented 6 years ago

I don't think we should rush to do this. I'm weakly opposed but will leave the decision to Michael and Luca. At the very least, it should be limited to just C++ files; e.g., the COPYRIGHT file must be an error.

bartgol commented 6 years ago

I think I involuntarily sparked this clang-format effort. I only wanted to address one comment of @mfdeakin-sandia in the diagnostic branch, then I thought clang would make it easier to fix all the spaces (rather than going throught the files by hand).

I don't see the need to do this quickly either. For one, I have not really looked at the clang-format file to check what it really does (except for noticing the Fortran-like 60 char lines). On the other hand, I'm not opposed to run it either. I guess I don't have a strong feeling in favor or against.

However, if it _does_go in, then we should run it before merging every PR. It makes no sense to run it once and then let the code format deteriorate with each PR...

mfdeakin-sandia commented 6 years ago

If we're doing this, I think the primary argument for doing this now is that we don't have many branches in flight now; after cam_fixes, the only active one I see is apply_cam_forcing, which consists of a single commit. I agree we should run this before every PR to avoid deterioration and conflicts; though we need to standardize on a version of clang-format, as otherwise it may change the format. I'd suggest the version shipped with clang 5.0.1; as it supports more of the options we want, and should be readily available for most platforms.