Page MenuHomePhabricator

[clang-format] Add IndentPragma style to eliminate common clang-format off scenario
Changes PlannedPublic

Authored by MyDeveloperDay on Dec 7 2020, 3:34 AM.

Details

Summary

A quick search of github.com, shows one common scenario for excessive use of //clang-format off/on is the indentation of #pragma's, especially around the areas of loop optimization or OpenMP

This revision aims to help that by introducing an IndentPragmas style, the aim of which is to keep the pragma at the current level of scope

    for (int i = 0; i < 5; i++) {
// clang-format off
        #pragma HLS UNROLL
        // clang-format on
        for (int j = 0; j < 5; j++) {
// clang-format off
            #pragma HLS UNROLL
            // clang-format on
     ....

can become

for (int i = 0; i < 5; i++) {
    #pragma HLS UNROLL
    for (int j = 0; j < 5; j++) {
        #pragma HLS UNROLL
    ....

This revision also support working alongside the IndentPPDirective of BeforeHash and AfterHash (see unit tests for examples)

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Dec 7 2020, 3:34 AM
MyDeveloperDay created this revision.

Should this be controlled on a case by case basis, maybe control indentation using a regex over the pragma arguments, WDYT?

clang/docs/ClangFormatStyleOptions.rst
1923–1924

Please add full stops at the end of these sentences.

1928
clang/include/clang/Format/Format.h
1533–1534

Please add full stops at the end of these sentences.

1536
clang/lib/Format/ContinuationIndenter.cpp
594

Full stop.

595–597

Can elide these braces.

clang/lib/Format/UnwrappedLineFormatter.cpp
1248

Capital letter at the start of comments.

Address review comments

MyDeveloperDay marked 7 inline comments as done.Dec 7 2020, 4:15 AM

Should this be controlled on a case by case basis, maybe control indentation using a regex over the pragma arguments, WDYT?

Most uses of pragmas seem to be at the 0'th level of scrope #pragma once etc

Most other pragmas are probably with the code (like comments) as such it seems we should add an option to keep it with the scope of the code, I think a regex is overkill, but we could add it later if deemed necessary.

This LGTM in general. I just have a doubt.
You've added messUp parameter to verifyFormat, because, IIUC, pragmas wouldn't be at the desired scope level indentation otherwise.
Shouldn't clang-format find out what the correct indentation level for pragmas should be instead of keeping the current indentation (when IndentPragmas: true)?
Correct me if my understanding is wrong.
Anyway, my point is that turning off messUp seems like a bad idea and may hide some problems, nope?

What would be the output of:

void foo() {
#pragma omp
  for (...) {
#pragma omp
    for (...) {
    }
  }
}

Do the #pragmas get intended to the level of corresponding fors or not?

MyDeveloperDay added a comment.EditedDec 7 2020, 6:16 AM

You've added messUp parameter to verifyFormat, because, IIUC, pragmas wouldn't be at the desired scope level indentation otherwise.

messUp was pulling the line below onto the pragma line, which was then unable to determine when if it should break it

i.e.

#pragma omp simd
for (int k = 0; k < 10; k++) {

became

#pragma omp simd for (int k = 0; k < 10; k++) {

I'm not sure why this occurred, but I assume there is no real token parsing of an #pragam line.

Add for(...) test case

Maybe it would be judicious to add a FIXME to test::messUp for the pragmas? Or fix it in this patch?

clang/unittests/Format/FormatTest.cpp
17647–17657

Actually I'd rather see a test were you mess up spaces manually (adding or removing spaces on pragma lines).
So instead of verifyFormat(formattedCode), I'd see EXPECT_EQ(formattedCode, format(lightly-messed-up-code, Style).

Allow the tests to messUp() the code, turns out its some sort of interaction ONLY with the BeforeHash case (last test)

MyDeveloperDay marked an inline comment as done.Dec 7 2020, 8:44 AM
curdeius accepted this revision.Dec 8 2020, 12:04 AM

LGTM with a single comment on the test.

clang/unittests/Format/FormatTest.cpp
17685–17693

Last nit, I'd mess up spaces like suggested here.

This revision is now accepted and ready to land.Dec 8 2020, 12:04 AM
MyDeveloperDay added inline comments.Dec 8 2020, 12:29 AM
clang/unittests/Format/FormatTest.cpp
17685–17693

This is BeforeHash not AfterHash, I think the test is correct (as it passes)

curdeius added inline comments.Dec 8 2020, 1:58 AM
clang/unittests/Format/FormatTest.cpp
17685–17693

Ok. My thought was that we were not really checking that this patch will correctly reformat pragmas that are not yet on correct columns, but after your change to remove messUp=false, we do test it.
Again, LGTM! And thank you for the patch (and the patience) :).

MyDeveloperDay marked 2 inline comments as done.Dec 8 2020, 2:26 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
17685–17693

Its never a problem @curdeius, I'm always happy to wait for your feedback as its considered and insightful, your comment made be go back and double check and that can never be a bad thing. Thanks for the accept.

RatTac reopened this revision.Jan 7 2021, 7:38 AM
RatTac added a subscriber: RatTac.

This is a great feature. However I found two issues:

First issue is with combination of IndentPPDirectives

// IndentPPDirectives: BeforeHash
// IndentPragmas: true

#if defined(WIN32)
	#define TEST // Correctly indented because IndentPPDirectives: BeforeHash
#endif

#if defined(WIN32)
#pragma warning(disable : 4005) // NOT indented even though IndentPPDirectives: BeforeHash
#endif

Second issue is with combination of lambda function:

inline void ompExecuteParallel(unsigned int endIndex, const std::function<void(unsigned int i)>& computeFunction, unsigned int startIdx)
{
	#pragma omp parallel for
	for(int i = (int)startIdx; i < (int)endIndex; i++)
		computeFunction(i);
}

int main()
{
	#pragma omp parallel for
	for(int i = 0; i < 10; i++)
	{
		// Correctly indented because IndentPragmas: true
		#pragma openmp critical
		{
			std::cout << i << endl;
		}
	}

	ompExecuteParallel(10, [&](unsigned int i) {
	#pragma openmp critical // Incorrectly indented even though IndentPragmas: true
		{
			std::cout << i << endl;
		}
	});

	return 0;
}

Hope this is the right place to raise concerns...

Regards
Tobias

This revision is now accepted and ready to land.Jan 7 2021, 7:38 AM

Sorry but due to the following issue raised by @RatTac , I'm reverting this prior to the LLVM 12 branch out so I can work on it further.

I also think this would impact #pragma's in any ifdef code which I envisage would be significant.

#if defined(WIN32)
#pragma warning(disable : 4005) // NOT indented even though IndentPPDirectives: BeforeHash
#endif
MyDeveloperDay planned changes to this revision.Jan 18 2021, 10:10 AM