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

git cl format and gcc disagree on "multi-line comment"s #48855

Closed
ghost opened this issue Apr 21, 2022 · 2 comments
Closed

git cl format and gcc disagree on "multi-line comment"s #48855

ghost opened this issue Apr 21, 2022 · 2 comments
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes.

Comments

@ghost
Copy link

ghost commented Apr 21, 2022

vm-kernel-gcc-linux is currently failing with:

 FAILED: obj/runtime/bin/ffi_test/libffi_test_functions.ffi_test_functions_vmspecific.o 
 g++ -MMD -MF obj/runtime/bin/ffi_test/libffi_test_functions.ffi_test_functions_vmspecific.o.d -DDART_SHARED_LIB -D_FORTIFY_SOURCE=2 -DDEBUG -I../../runtime -I../.. -Igen -fPIC -m32 -msse2 -mfpmath=sse -fno-exceptions -Wall -Wextra -Werror -Wendif-labels -Wno-missing-field-initializers -Wno-unused-parameter -Wno-ignored-qualifiers -fdebug-prefix-map=/b/s/w/ir/cache/builder/sdk=../.. -no-canonical-prefixes -O2 -fdata-sections -ffunction-sections -g3 -ggdb3 -fvisibility-inlines-hidden -fno-omit-frame-pointer -std=c++17 -fno-rtti -fno-exceptions -c ../../runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc -o obj/runtime/bin/ffi_test/libffi_test_functions.ffi_test_functions_vmspecific.o
 ../../runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc:1304:9: error: multi-line comment [-Werror=comment]
  #endif  // defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID) ||     \
          ^
 cc1plus: all warnings being treated as errors

https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8816300984185262753/+/u/build_dart_ia32/stdout

But modifying the code to:

#endif  // defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID) ||
        // defined(DART_HOST_OS_MACOS)

Will cause git cl format (and therefore our presubmit) to complain.

I'm guessing we'll need to modify git cl format to not enforce multiline comments like this.

@ghost ghost added the area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. label Apr 21, 2022
@whesse
Copy link
Contributor

whesse commented Apr 21, 2022

git cl format is part of depot_tools, so it would need to be modified there. It pulls down the latest version of clang-format binaries and runs them to format the C++ code according to clang standards.

I think rather than trying to exclude this file from clang formatting, it would be better for our build rules to disable the gcc warning for multi-line comments when building with gcc.

If clang standards and gcc warnings are conflicting, I think we will want to go with clang standards.
We should be able to disable this warning by adding it here, or 10 lines later:

https://github.com/dart-lang/sdk/blob/main/build/config/compiler/BUILD.gn#L572

@whesse
Copy link
Contributor

whesse commented Apr 21, 2022

Also, modifying the code to:

#endif  // defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID) ||
// defined(DART_HOST_OS_MACOS)

or to

#endif /* defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID) ||
          defined(DART_HOST_OS_MACOS) */

will avoid the problem.
If the second comment line is indented at all, then clang-format will first indent it the same as the first line, then, the next time it is run, add the backslash.

Other alternatives are to put all comments on the succeeding lines, none on the line with #endif.

I have filed an issue with the team that maintains clang-format for Chromium, and they may upstream it to clang-format at
https://github.com/llvm/llvm-project/labels/clang-format

I filed the issue as low-priority, since there seems to be workarounds.

copybara-service bot pushed a commit that referenced this issue Apr 21, 2022
TEST=comment only change

Bug: #48855
Change-Id: I39ea0593d2e660803b2e6520f51c2a1c9aaaaec7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/241967
Auto-Submit: Clement Skau <cskau@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes.
Projects
None yet
Development

No branches or pull requests

2 participants