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

expand tabs #521

Merged
merged 12 commits into from
Sep 5, 2021
Merged

expand tabs #521

merged 12 commits into from
Sep 5, 2021

Conversation

urbanjost
Copy link
Contributor

No description provided.

@urbanjost
Copy link
Contributor Author

minimal changes to expand tabs before searching for INCLUDE and USE statements

Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Thanks for separating that out again. This all looks good 👍

@urbanjost
Copy link
Contributor Author

this was refactored to simplify it from #510 per a request from @LKedward, but more importantly closes #508. The basic change is very simple, with a little complication to reduce the number of calls to LOWER(3f) by a factor of ten, as suggested by The OP for #508. To reintroduce some of the other parts that emerged originally in #510 (a fully implemented verbose mode, replacing STOP with a function to standardize the behavior and appearance of a STOP including only doing an ERROR STOP in verbose mode, possibly adding some color, a less verbose normal build...) this needs to progress or the other proposals/fixes will have a lot of conflicts. Anyone have time to review? @awvwgk -- you had looked at the previous (more complicated) #510 version?

@LKedward LKedward requested a review from awvwgk July 28, 2021 09:13
So I put the orginal .gitignore back in, and even made it bigger if
"fpm new -full" is used. Here is why ...

Although I preferred only auto-generating .gitignore in build/ initially,
after reviewing the comments I built a list of public github projects
containing fpm.toml files and pulled them and eliminated most trivial
ones that appeared to just be experiments in using fpm by somewhat
arbitrarily removing those with less than one hundred lines of Fortran
source and found the majority of those remaining had a customized (ie. not
the default one built by the "fpm new" subcommand) .gitignore file;
indicating many projects need a .gitignore file in the top directory,
so might as well keep seeding it(?),

I found seven that did not have a .gitignore at all, reinforcing the
need to put one in the build directory itself, as this still does. For
the most part those appeared to be unintentional and looked like several
were inadvertently storing binaries on github as a result, indicating
the change to making a build/.gitignore file is a good idea.

stdlib-fpm does not have one, but did not have a build/ directory on
github, but the change to put one in build/ itself will prevent that being
a problem for anyone that forks or clones it, so that is covered now.

I found a pattern where it looks like a number of large sites are
setting up to be able to build with other systems other than fpm(1),
which by itself does not need a .gitignore file of any complexity;
just perhaps skipping non-core files like logs and fodder and other
user-specific needs it is not too easy to optimize except by exclusion
(that is, exclude everything except the default files used by fpm,
which does not feel like a good direction).

On the other hand, I found two other others using the default github list
for Fortran projects, and saw several other projects essentially making
their own attempt at the same list, but I think a large .gitignore file
is conflating for a new (and probably typical) user so I put back the
simple one by default when "fpm new" is used, but if "fpm new -full"
is used, add the github recommendations.  The github recommendations
do not directly benefit a pure fpm project, but at least so far that is
still a minority of the sample packages I found; so adding it at all or
adding it via --full is a bit kluge, but seemed like it would make handy
information readily available if perhaps not in the most intuitive way,
but would not conflate basic usage.
@awvwgk
Copy link
Member

awvwgk commented Jul 31, 2021

Since we only need to cope with tabs when parsing, can't we safely replace them one to one with spaces? The parsed lines are only used internally in fpm, therefore a simple replacement might be sufficient here.

@urbanjost
Copy link
Contributor Author

Already had the routine for expanding tabs, tried simple spaces versus expanding and the timing on a package with over 1 000, source files and > 100 000, lines of source it made no difference, and it is also used as input to the checksum and there are some edge cases where the tabs could be inside of string constants that that could decide incorrectly not to rebuild without expansion; admittedly not likely to be a common issue but since it did not cause a performance issue (and like I mentioned, I have had expand for a long time ( it was first used for this purpose (removing tabs from Fortran source files because we were using several compilers that did not accept them in input files)! so it seemed worth the almost-no-cost performance difference to eliminate the issue. I would prefer to do the expansion to the common eight-stop format as-is because of that.

@urbanjost
Copy link
Contributor Author

Odd. My browser indicates there are now conflicts but will not let me view them.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

This looks ready to go for me once the merge conflict is fixed.

@urbanjost
Copy link
Contributor Author

urbanjost commented Sep 5, 2021

This is the third refactoring where it originally had no conflicts and when I updated my fork from the master I did not get any conflicts and the web page says the changes are to big to show them. I do not want to start the approval process over again with a new pull request like last time. What commands are needed to see the conflicts. I have made two different pulls and they all show it without conflicts.

NEVER MIND -- the changes just took an unusual amount of time to show up

@awvwgk
Copy link
Member

awvwgk commented Sep 5, 2021

Thanks, I'll go ahead and merge now.

@awvwgk awvwgk merged commit 5c908fc into fortran-lang:master Sep 5, 2021
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.

3 participants