Page MenuHomePhabricator

Allow additional file suffixes/extensions considered as source in main include grouping
ClosedPublic

Authored by furdyna on Sep 19 2019, 1:41 AM.

Details

Summary

By additional regex match, grouping of main include can be enabled in files that are not normally considered as a C/C++ source code.
For example, this might be useful in templated code, where template implementations are being held in *Impl.hpp files.
On the occassion, 'assume-filename' option description was reworded as it was misleading. It has nothing to do with style=file option and it does not influence sourced style filename.

Diff Detail

Event Timeline

furdyna created this revision.Sep 19 2019, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2019, 1:41 AM
furdyna edited the summary of this revision. (Show Details)Sep 19 2019, 1:43 AM

Hi, @krasimir @sylvestre.ledru , would you be able to help me with this patch? I am looking for anyone interested in reviewing this :)

MyDeveloperDay requested changes to this revision.Oct 24 2019, 6:35 AM
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a subscriber: MyDeveloperDay.

I think fundamentally this seems like a reasonable idea, for those that don't need they don't need to use it.

clang/docs/ClangFormat.rst
32–33

I agree this seems wrong, it's making it sound like -assume-filename is a file that will replace .clang-format .. I don't think thats true as you correctly point out

clang/lib/Format/Format.cpp
480

I think we are missing a change to the operator== in Format.h (I notice IncludeIsMainRegex is missing from there too)

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
183

whats the default value of IncludeIsMainSourceRegex?, are we making a regex even when its empty?

could we not or the regex after if its defined

IsMainFile = FileName.endswith(".c") || FileName.endswith(".cc") ||
               FileName.endswith(".cpp") || FileName.endswith(".c++") ||
               FileName.endswith(".cxx") || FileName.endswith(".m") ||
               FileName.endswith(".mm");
if (!Style.IncludeIsMainSourceRegex.empty()){
    llvm::Regex MainFileRegex(Style.IncludeIsMainSourceRegex);
    IsMainFile |= MainFileRegex.match(FileName);    
}
clang/tools/clang-format/ClangFormat.cpp
80

This may be a little out of date and needs rebasing as I believe this reformat has already been done, but your new wording is correct

This revision now requires changes to proceed.Oct 24 2019, 6:35 AM
furdyna updated this revision to Diff 228872.Nov 12 2019, 4:58 AM
furdyna marked 2 inline comments as done.

Hi @MyDeveloperDay, I changed the code following your advice.

Thanks for cathing the operator==(), I was not sure if this omission was intentional or not - fixed both comparisons.

MyDeveloperDay accepted this revision.EditedNov 12 2019, 6:06 AM

Thanks for the patch,

This LGTM, just a small nit for the future, can you ensure the diff is a full context diff with something like @git diff --cached -U999999 > patch.diff (I wanted to check you got the recent changes by @sylvestre.ledru in docs/ClangFormat.rst) but I couldn't scroll that far down, so you'll need to be sure you've rebased if you haven't done so already.

This revision is now accepted and ready to land.Nov 12 2019, 6:06 AM

@MyDeveloperDay Thanks for the review!

I did rebase it a few hours ago. I actually made sure to use the full-context diff (with -U999999 option) - I even have this as a last command in my command history, and the patch file on my disk is full-context. So either I still missed something, or the tool is playing with us ;)

BTW, I don't have rights to commit to master / merge the pull request. Can you do this for me?

This revision was automatically updated to reflect the committed changes.