Skip to content

Conversation

NlightNFotis
Copy link
Contributor

This silences a CI warning by one of our auxiliary runs, which
checks whether an include inside a file is used.

This change adds the rationale as a comment, and it also adds
a pragma directive for the tool to silence the warning.

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@@ -12,6 +12,14 @@ class goto_modelt;
class message_handlert;
class optionst;

// There's has been a design decision to allow users to include all of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick: spurious 's

Comment on lines 22 to 23
// IWYU pragma: keep
#include "options.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you need to put the comment on the same line as the include.

@NlightNFotis
Copy link
Contributor Author

Thanks @tautschnig, both of the comments have been addressed.

@thomasspriggs
Copy link
Contributor

@tautschnig Include what you use is still failing. The line it appears to be complaining about isn't even an include?!

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Base: 78.36% // Head: 78.38% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (1496828) compared to base (f025c91).
Patch coverage: 79.06% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7418      +/-   ##
===========================================
+ Coverage    78.36%   78.38%   +0.01%     
===========================================
  Files         1651     1651              
  Lines       190009   190045      +36     
===========================================
+ Hits        148896   148961      +65     
+ Misses       41113    41084      -29     
Impacted Files Coverage Δ
src/analyses/constant_propagator.h 82.35% <ø> (ø)
src/analyses/invariant_set.cpp 0.00% <0.00%> (ø)
src/analyses/invariant_set.h 0.00% <0.00%> (ø)
src/ansi-c/c_expr.h 100.00% <ø> (ø)
src/ansi-c/c_typecheck_base.h 100.00% <ø> (ø)
src/ansi-c/scanner.l 63.39% <ø> (ø)
src/cpp/cpp_item.h 64.15% <0.00%> (ø)
src/ansi-c/ansi_c_convert_type.cpp 79.48% <8.16%> (ø)
.../src/java_bytecode/character_refine_preprocess.cpp 54.27% <28.57%> (ø)
src/analyses/local_may_alias.cpp 63.80% <33.33%> (ø)
... and 146 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@NlightNFotis
Copy link
Contributor Author

Adding the error in the CI here because I will implement a change to suppress it, in case people want to have a look:

/home/runner/work/cbmc/cbmc/src/libcprover-cpp/api.h should remove these lines:
[49553](https://github.com/diffblue/cbmc/actions/runs/3647317139/jobs/6159408798#step:5:49554)
- class optionst;  // lines 13-13
[49554](https://github.com/diffblue/cbmc/actions/runs/3647317139/jobs/6159408798#step:5:49555)
Unnecessary includes found. Use '// IWYU pragma: keep' to override this.

@tautschnig
Copy link
Collaborator

@tautschnig Include what you use is still failing. The line it appears to be complaining about isn't even an include?!

Well, yes, include-what-you-use also helps you with forward declarations. class optionst; indeed is unnecessary, because options.h (which remains included per the new comment) already provides for the same forward declaration.

@thomasspriggs
Copy link
Contributor

@tautschnig Include what you use is still failing. The line it appears to be complaining about isn't even an include?!

Well, yes, include-what-you-use also helps you with forward declarations. class optionst; indeed is unnecessary, because options.h (which remains included per the new comment) already provides for the same forward declaration.

Ah. I hadn't realised it examined forward declarations as well. If we remove the forward declaration of optionst in api.h, then that will be introducing a transitive dependency. api.h requires the forward declaration of optionst because of the declaration on line 33 - std::unique_ptr<optionst> options. Therefore if remove the forward declaration then we would be removing something which is used. This is not good for maintenance, because if we were to change api_optionst later so that it no longer requires the forward declaration then we can't actually remove it from src/libcprover-cpp/options.h without adding it back to api.h.

I am not sure "it still compiles if I remove this" is a sufficiently good test in this case. Removing the forward declaration will save no compilation time and add maintenance work.

@tautschnig
Copy link
Collaborator

@tautschnig Include what you use is still failing. The line it appears to be complaining about isn't even an include?!

Well, yes, include-what-you-use also helps you with forward declarations. class optionst; indeed is unnecessary, because options.h (which remains included per the new comment) already provides for the same forward declaration.

Ah. I hadn't realised it examined forward declarations as well. If we remove the forward declaration of optionst in api.h, then that will be introducing a transitive dependency. api.h requires the forward declaration of optionst because of the declaration on line 33 - std::unique_ptr<optionst> options. Therefore if remove the forward declaration then we would be removing something which is used. This is not good for maintenance, because if we were to change api_optionst later so that it no longer requires the forward declaration then we can't actually remove it from src/libcprover-cpp/options.h without adding it back to api.h.

I am not sure "it still compiles if I remove this" is a sufficiently good test in this case. Removing the forward declaration will save no compilation time and add maintenance work.

We have probably spent way too much time debating a piece of code that might look quite different a week from now. Even if it were to be more stable than this: everyone should feel free to override what the include-what-you-use tool says. All that I wanted to make sure when adding this CI task was that we don't have completely useless includes that are unintentionally left behind.

@NlightNFotis NlightNFotis merged commit 753df0a into diffblue:develop Dec 8, 2022
@NlightNFotis NlightNFotis deleted the fix_api_options branch December 8, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants