Skip to content

Revive compiler warnings#278

Merged
lan496 merged 5 commits intospglib:developfrom
lan496:add-compiler-warnings
Jun 10, 2023
Merged

Revive compiler warnings#278
lan496 merged 5 commits intospglib:developfrom
lan496:add-compiler-warnings

Conversation

@lan496
Copy link
Copy Markdown
Member

@lan496 lan496 commented Jun 3, 2023

@lan496 lan496 requested review from atztogo and removed request for atztogo June 3, 2023 01:32
@lan496 lan496 marked this pull request as draft June 3, 2023 01:38
@lan496 lan496 force-pushed the add-compiler-warnings branch from dc4b510 to f5d5efe Compare June 3, 2023 02:15
@atztogo
Copy link
Copy Markdown
Collaborator

atztogo commented Jun 3, 2023

pre-commit fails due to clang-format. @lan496, previously clang-format didn't check following lines?

-    int (*steps[8])(NiggliParams *p) = {step1, step2, step3, step4,
-                                        step5, step6, step7, step8};
+    int (*steps[8])(NiggliParams * p) = {step1, step2, step3, step4,
+                                         step5, step6, step7, step8};

@lan496
Copy link
Copy Markdown
Member Author

lan496 commented Jun 3, 2023

@atztogo It seems. In my local environment (clang-format==16.0.4), formatted to int (*steps[8])(NiggliParams *p), but CI's clang-format complains about it.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (3800c46) 85.92% compared to head (0733349) 85.92%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #278      +/-   ##
===========================================
- Coverage    85.92%   85.92%   -0.01%     
===========================================
  Files           23       23              
  Lines         6082     6081       -1     
===========================================
- Hits          5226     5225       -1     
  Misses         856      856              
Flag Coverage Δ
c_api 74.21% <100.00%> (-0.01%) ⬇️
fortran_api 37.38% <0.00%> (+<0.01%) ⬆️
python_api 82.84% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/kgrid.c 94.28% <ø> (ø)
src/spglib.c 73.00% <ø> (-0.04%) ⬇️
src/kpoint.c 97.81% <100.00%> (ø)
src/niggli.c 85.27% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lan496 lan496 marked this pull request as ready for review June 3, 2023 02:22
@lan496 lan496 requested a review from atztogo June 3, 2023 02:22
@lan496
Copy link
Copy Markdown
Member Author

lan496 commented Jun 3, 2023

@LecrisUT It seems the versions of clang-format in my environment and CI are diffenrent. The CI's clang-format will be handled at https://github.com/Takishima/cmake-pre-commit-hooks. Do you know how to know and specify the clang-format version in CI?

@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Jun 3, 2023

@atztogo It seems. In my local environment (clang-format==16.0.4), formatted to int (*steps[8])(NiggliParams *p), but CI's clang-format complains about it.

Oh yeah I had this issue before. We can use pre-commit-ci to get newer versions and/or use a container with pre-installed modern clang-format.

Also have to check if we can we can enforce a clang-format minimum version.

add_compile_options(/W4)
else()
add_compile_options(-Wall -Wextra -Wpedantic)
endif()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How many warnings are there to address. Is it eventually feasible to convert the warnings to errors for the CI?

@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Jun 3, 2023

@LecrisUT It seems the versions of clang-format in my environment and CI are diffenrent. The CI's clang-format will be handled at https://github.com/Takishima/cmake-pre-commit-hooks. Do you know how to know and specify the clang-format version in CI?

Oh, I didn't know there is a PyPI version of clang-format as well (reference). Maybe can specify the version there.

@lan496 lan496 force-pushed the add-compiler-warnings branch from f5d5efe to 49516a0 Compare June 3, 2023 11:24
@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Jun 7, 2023

At

- repo: https://github.com/Takishima/cmake-pre-commit-hooks
rev: v1.5.3
hooks:
- id: clang-format

Try:

 - repo: https://github.com/Takishima/cmake-pre-commit-hooks 
   rev: v1.5.3 
   hooks: 
     - id: clang-format 
       additional_dependencies: [ clang-format >= 16 ]

@lan496
Copy link
Copy Markdown
Member Author

lan496 commented Jun 7, 2023

OK, it seems the CI uses clang-format>=16 now! I'll clean up the commits.

@lan496 lan496 force-pushed the add-compiler-warnings branch from ffb100c to 7373cc0 Compare June 7, 2023 11:59
@LecrisUT
Copy link
Copy Markdown
Collaborator

LecrisUT commented Jun 7, 2023

Add to todo:

  • warning regarding passing non-const array to const array. That should be perfectly fine, but why does the compiler trigger a warning about it
  • uninitialized warnings should be fixed
  • make warnings become error: CMAKE_COMPILE_WARNING_AS_ERROR

@atztogo
Copy link
Copy Markdown
Collaborator

atztogo commented Jun 10, 2023

@LecrisUT, can we merge this PR to merge #278 ?

Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT left a comment

Choose a reason for hiding this comment

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

There is one issue to address. Why are we putting this in the global cmakelists and not the CI? This one will enforce warning compilation of downstream users. Alternatively use target_compile_option(PRIVATE).

Also, rebase so that CI passes

lan496 added 5 commits June 10, 2023 14:20
Note: The input `max_size` is removed from `get_symmetry_with_site_tensors`
because this input is not used in the function.
@lan496 lan496 force-pushed the add-compiler-warnings branch from 7373cc0 to 0733349 Compare June 10, 2023 05:21
@lan496
Copy link
Copy Markdown
Member Author

lan496 commented Jun 10, 2023

@LecrisUT I've fixed to use target_compile_option(PRIVATE): 5086f74

@LecrisUT
Copy link
Copy Markdown
Collaborator

Ok, the final issue is about my last comment

Add to todo:

* warning regarding passing non-const array to const array. That should be perfectly fine, but why does the compiler trigger a warning about it

* uninitialized warnings should be fixed

* make warnings become error: [`CMAKE_COMPILE_WARNING_AS_ERROR`](https://cmake.org/cmake/help/latest/variable/CMAKE_COMPILE_WARNING_AS_ERROR.html#variable:CMAKE_COMPILE_WARNING_AS_ERROR)

Can you make an issue about it if we are not addressing these issues in this PR?

@lan496
Copy link
Copy Markdown
Member Author

lan496 commented Jun 10, 2023

Sure! I've opened the issue #289.

@lan496 lan496 merged commit 94d288f into spglib:develop Jun 10, 2023
@lan496 lan496 deleted the add-compiler-warnings branch June 10, 2023 05:49
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.

4 participants