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

Build with -pedantic, fix problems #3892

Merged
merged 9 commits into from
Feb 16, 2023
Merged

Build with -pedantic, fix problems #3892

merged 9 commits into from
Feb 16, 2023

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Feb 15, 2023

I think that -pedantic would be beneficial compilation flag on top of the existing warnings. Some positives and negatives:

  • + it catches some edge cases that are ignored by gcc by default, e.g. operators defined both as member and as global function (if the class is not templated)
  • + we should follow standard C++
  • + it should make compilation by e.g. clang easier (which enables use of tools such as clang-tidy)
  • - it could be too strict (e.g. designated initalizers, see below)
  • - even this does not guarantee that the code is purely according to standard

This PR enables it and fixes the problems that -pedantic has with the current code:

  • most often these are semicolons that should not be present (e.g. at end of namespaces)
  • also badly-escaped string literals
  • designated initializers (e.g. {.foo = 4, .bar = 5}), this is arguably the only one that potentially matters
    • designated initializers are in C, but not in C++ (until C++20, which actually introduced them, I think that slightly restricted compared to the C version)
    • there seems to be no way to enable both -pedantic and designated initializers in C++17 (i.e. there is no fine-grained control)
    • however, there is only one occurrence in p4c (as far as I / the build I've been able to do can see), so the impact is pretty small.

Overall, I think the benefits of -pedantic overturn its disadvantages, but of course I can barely propose this :-).

Copy link
Contributor

@davidbolvansky davidbolvansky left a comment

Choose a reason for hiding this comment

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

LGTM.

Alternatively we could introduce a new option, like ENABLE_PEDANTIC_BUILD, on by default, to control this, but personally I dont think it is worth it; similar changes required by -pedantic in bf-p4c were quite small.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 15, 2023

Apparently some checks that run here are not too easy to run locally, I'll go through the problems and see if they can be resolved.

Copy link
Contributor

@grg grg left a comment

Choose a reason for hiding this comment

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

Looks okay to me as well, although the loss of designated initializers is a little unfortunate 🙁

I'd wait to see what feedback Chris and/or Mihai have to say about this change before proceeding.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 15, 2023

Looks like the Ubuntu 18 build is using GCC 7.5:

-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0

But according to dependencies we require "A C++17 compiler. GCC 9.1 or later or Clang 6.0 or later is required.". So this should probably be changed in Ubuntu 18 build.

lib/exceptions.h Outdated Show resolved Hide resolved
@mihaibudiu
Copy link
Contributor

Is anyone working to fix the Mac build?

@davidbolvansky
Copy link
Contributor

Please rebase

@grg
Copy link
Contributor

grg commented Feb 15, 2023

Is anyone working to fix the Mac build?

@mbudiu-vmw Is it failing to build, or is it building incorrectly and producing errors when you test? I tried just now and it built fine, but 70 tests fail. I did not pass any options to cmake.

@mihaibudiu
Copy link
Contributor

All recent PRs e.g., #3889 are failing due to the MacOS CI build, e.g.: https://github.com/p4lang/p4c/actions/runs/4186877650/jobs/7255975949
The error looks like:

 CMake Error at /usr/local/Cellar/cmake/3.25.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Protobuf (missing: Protobuf_LIBRARIES) (found suitable
  version "3.21.12", minimum required is "3.0.0")
Call Stack (most recent call first):
  /usr/local/Cellar/cmake/3.25.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
-- Configuring incomplete, errors occurred!
  /usr/local/Cellar/cmake/3.25.2/share/cmake/Modules/FindProtobuf.cmake:650 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
See also "/Users/runner/work/p4c/p4c/build/CMakeFiles/CMakeOutput.log".
  CMakeLists.txt:117 (find_package)
See also "/Users/runner/work/p4c/p4c/build/CMakeFiles/CMakeError.log".

@vlstill
Copy link
Contributor Author

vlstill commented Feb 16, 2023

I'll rebase this after #3896 is merged to fix the macOS build.

@vlstill vlstill merged commit 19ed58d into main Feb 16, 2023
@vlstill vlstill deleted the vstill/pedantic branch February 16, 2023 18:01
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.

6 participants