This is an archive of the discontinued LLVM Phabricator instance.

Reformat inferior's main.cpp in lldb-server test
ClosedPublic

Authored by eugene on Feb 21 2017, 4:01 PM.

Details

Summary

main.cpp is complete mess of tabs and spaces. This change brings it to compliance with LLVM coding style.

Diff Detail

Repository
rL LLVM

Event Timeline

eugene created this revision.Feb 21 2017, 4:01 PM
jmajors accepted this revision.Feb 21 2017, 4:07 PM

Do we have any plans to put something like cpplint into effect?

This revision is now accepted and ready to land.Feb 21 2017, 4:07 PM

Do we have any plans to put something like cpplint into effect?

First step would be to have warning free build.
Then we can turn "warnings as errors" on.

It looks like clang-format wasn't run over this file as it was over all the main lldb sources in the infamous universal code reformatting. That seems odd.

Anyway, it might be better just to do that to this file using the top level .clang-format. Note that you are making several choices which were not the choices made by clang-format using the .clang-format file that was used to implement this reformatting. We probably shouldn't revise that decision piecemeal.

packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
32–42 ↗(On Diff #89296)

This change seems a shame, the original was much easier to read.

62 ↗(On Diff #89296)

clang-format moved the initial { for functions into the function definition line universally when it was run over the lldb sources. If we want to revise that decision, and go back to the initial function curly starting a line, that would be fine by me, but I don't think we should do it piecemeal.

Ditto for separating the return type & function name onto separate lines. That was the way we did it originally, but the clang-format style that was chosen for the reformatting undid that. I much prefer the way you changed it to here, but that's a decision we should make globally, not file by file.

Actually, we didn't run the clang-format over the test case source files, since we didn't want to have to deal with the possible failures resulting. So just clang-formatting this file is probably the right way to go.

eugene updated this revision to Diff 89311.Feb 21 2017, 6:02 PM
eugene updated this revision to Diff 89312.Feb 21 2017, 6:09 PM

Anyway, it might be better just to do that to this file using the top level .clang-format. Note that you are making several choices which were not the choices made by clang-format using the .clang-format file that was used to implement this reformatting. We probably shouldn't revise that decision piecemeal.

It wasn't my intention to revise any formatting decisions made earlier.
I just ran 'clang-format -style=file -i main.cpp' assuming that it will actually make the file complaint with the lldb's coding style by using top level .clang-format.

Is there any document other than http://llvm.org/docs/CodingStandards.html describing LLDB coding conventions?

eugene marked an inline comment as done.Feb 21 2017, 6:22 PM
eugene added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
32–42 ↗(On Diff #89296)

Agree. But I think consistency is better than beauty in case of code formatting.

62 ↗(On Diff #89296)

Could you please elaborate.
I just ran 'clang-format -style=file -i main.cpp' assuming that it will actually make the file complaint with the lldb's coding style by using top level .clang-format.

krytarowski added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
74 ↗(On Diff #89312)

Note to self -

#elif defined(__NetBSD__)
printf("%" PRIx64, static_cast<uint64_t>(_lwp_self()));

and include

#if defined(__NetBSD__)
#include <lwp.h>
#endif
labath edited edge metadata.Feb 22 2017, 1:50 AM

clang-format did not pick up the correct formatting because we have a .clang-format file in packages/Python/lldbsuite. This was necessary because a lot of the tests would get broken, as the formatting would break our // place breakpoint here annotations (also, stepping behavior is affected by the line breaks). It would be an interesting project to format all existing tests, but this would probably need to be done on a test-by-test basis (and may involve adding additional clang-format directives like CommentPragmas, ...).

That said, none of these problems should apply to lldb-server tests as they operate differently, so reformatting them is probably a good idea. I guess you should also then put a .clang-format file in the lldb-server test folder to make sure it stays formatted in the future.

eugene updated this revision to Diff 89404.Feb 22 2017, 12:23 PM

I have added local .clang-format file. But it didn't change the way main.cpp was formated.

labath accepted this revision.Feb 23 2017, 1:51 AM

I have added local .clang-format file. But it didn't change the way main.cpp was formated.

That's fine, I did not expect that to make a difference now. What it will do though, is make sure that any subsequent modifications conform to the formatting rules (if one runs e.g. "git clang-format"). Without this, it would pick up the .clang_format file in the parent test folder which says "don't touch anything".

LGTM.

This revision was automatically updated to reflect the committed changes.