flang-compiler / f18

F18 is a front-end for Fortran intended to replace the existing front-end in the Flang compiler
229 stars 48 forks source link

[LLVMify F18] Compiler module folders should have capitalised names #980

Closed CarolineConcatto closed 4 years ago

CarolineConcatto commented 4 years ago

This patch renames the modules in f18 to use a capital letter in the module name[1] Firstly we create new folders with the capitalised name of the modules and then we do git mv for these new folders. Besides that, many files were modified to point and include the names of these modules.

The task is very easy, the only problem is that it will mess with git history thinking that new files were created, and lots of them were deleted. So to reduce to chaos, this task will be split into two commits. The first one creates and moves the files inside f18. The second commit will update the includes in the files that need to be updated.

This first commit renames the folder inside: ->include/flang/[common | decimal | evaluate | parser | semantics] ->lib/[common | decimal | evaluate | parser | semantics] ->test/[decimal | evaluate | preprocessing | runtime | semantics] ->test-lit/driver to capitalise names

This is achieved by doing git mv for each module, as shown below: $for i in lib/common/*; do git mv $i lib/Common; done

[1]https://github.com/flang-compiler/f18/issues/963

Signed-off-by: Caroline Concatto caroline.concatto@arm.com

CarolineConcatto commented 4 years ago

Question: should the namespaces also need to be changed? Like: namespace Fortran::common { namespace Fortran::decimal {

CarolineConcatto commented 4 years ago

This branch needs to rebase with origin/master to remove the conflicts. I did try that, but git push seems to need --force. ($git push myfork HEAD --force) Is that correct? Or I am doing something wrong?

CarolineConcatto commented 4 years ago

Ok, after talking with @DavidTruby I think it was safe to do a git pull --rebase and a git push force. The push has only the rebase with master and no changes in the code.

CarolineConcatto commented 4 years ago

There is some problem with the merge.
Mainly with the file test/semantics/resolve70.f90.

CarolineConcatto commented 4 years ago

@RichBarton-Arm I had this conversation with @peterwaller-arm before deciding what to do. I am going to paste AS IS here our conversation. Hope @peterwaller-arm doesn't mind Hey Peter, I have a problem with git history, maybe you could help. This ticket here is to Capitalise the modules if f18.(https://github.com/flang-compiler/f18/issues/963) I do git mv and after that git add in the files that needed to be updated. git add in the modified files in the old folder shows these files as deleted and the files in the new folder as new. This only happens to the files that were modified. The suggestion online is to do two commits one with only git mv and a second commit git add in the modified files. Question is: I will probably be asked to squash the 2 commits. Will the problem described now be shown again if I do git squash?

Peters answer: No, squashing will do the right thing. I recommend doing the changes as two commits initially - that's a good suggestion from whoever said it. I'm not sure exactly what you did from your description. It sounds like you're moving files and modifying them.

DavidTruby commented 4 years ago

Used the patented 'eye tool' on this one and only spotted a couple of things.

I don't think splitting this into two in the way that you have done is right because I think the first commit won't build without the second one. I get wanting to split this up but that shouldn't be at the expense of making each commit leave the repo in a buildable state. We should squash the commits before we merge the PR.

The reason for splitting this up isn't that we want to, but git just simply doesn't understand that a move has happened if you do both in one commit, and there doesn't seem to be a way to make it understand that other than splitting the commits. So we have a choice between a rogue commit that doesn't build or a broken history because git sees this as every file in the repo being deleted and new files being added. I personally think the latter is worse behaviour than the former!

CarolineConcatto commented 4 years ago

[Out-of-topic]I noticed that there is no header text inside all .cpp and .h files inside f18/test. Is this correct?

RichBarton-Arm commented 4 years ago

[Out-of-topic]I noticed that there is no header text inside all .cpp and .h files inside f18/test. Is this correct?

I think that follows the pattern in other LLVM projects too. Sometimes the tests are very small files, so they would mostly be header. Don't think that needs to be part of this patch anyhow.

RichBarton-Arm commented 4 years ago

@sscalpone we think this is ready to land. We need to squash before merging. Does anyone else want to review this before we squash?

sscalpone commented 4 years ago

@CarolineConcatto @RichBarton-Arm Looks good. Please resolve conflicts & squash.

sscalpone commented 4 years ago

@DavidTruby There are still conflict to resolve.