Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 25, 2020

Conversation

CarolineConcatto
Copy link
Collaborator

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]#963

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

@CarolineConcatto
Copy link
Collaborator Author

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

@CarolineConcatto
Copy link
Collaborator Author

CarolineConcatto commented Feb 13, 2020

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
Copy link
Collaborator Author

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.

Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change here too.
test/Evaluate/CMakeLists.txt:#===-- test/evaluate/CMakeLists.txt ----------------------------------------===#

Copy link
Contributor

@RichBarton-Arm RichBarton-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

On the clang-format change Peter pointed out a way that we can hide commits like this from git history. So we can use that on this commit too.

documentation/ImplementingASemanticCheck.md Outdated Show resolved Hide resolved
lib/Evaluate/characteristics.cpp Show resolved Hide resolved
@CarolineConcatto
Copy link
Collaborator Author

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

@CarolineConcatto
Copy link
Collaborator Author

@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.(#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
Copy link
Collaborator

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!

Copy link
Contributor

@RichBarton-Arm RichBarton-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - happy with the justification on the split commits. It sounds like the plan is to squash before merge anyway.

lib/Evaluate/characteristics.cpp Show resolved Hide resolved
@CarolineConcatto
Copy link
Collaborator Author

[**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
Copy link
Contributor

[**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.

Copy link
Contributor

@RichBarton-Arm RichBarton-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the last two changes and responses to my other comments, I am happy with the patch.

@RichBarton-Arm
Copy link
Contributor

@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
Copy link
Member

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

Copy link
Collaborator

@tskeith tskeith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message is 120 lines long. Can you edit it down to something reasonable?

@DavidTruby DavidTruby force-pushed the rename_modules branch 2 times, most recently from c5af1c7 to 500f81a Compare February 19, 2020 10:16
@sscalpone
Copy link
Member

@DavidTruby There are still conflict to resolve.

#include "llvm/Support/raw_ostream.h"
#include "flang/Common/Fortran-features.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the llvm file above come at the end of the listing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should. For some reason git thought it would be nice to put at the beginning, once all this patch does is sed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

This patch renames the modules in f18 to use a capital letter in the
module name

Signed-off-by: Caroline Concatto <caroline.concatto@arm.com>
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sscalpone sscalpone merged commit d2eb7a1 into flang-compiler:master Feb 25, 2020
swift-ci pushed a commit to apple/llvm-project that referenced this pull request Apr 9, 2020
… names (flang-compiler/f18#980)

This patch renames the modules in f18 to use a capital letter in the
module name

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

Original-commit: flang-compiler/f18@d2eb7a1
Reviewed-on: flang-compiler/f18#980
@CarolineConcatto CarolineConcatto deleted the rename_modules branch August 6, 2020 15:53
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
… names (flang-compiler/f18#980)

This patch renames the modules in f18 to use a capital letter in the
module name

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

Original-commit: flang-compiler/f18@d2eb7a1
Reviewed-on: flang-compiler/f18#980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants