Skip to content

Conversation

@giltho
Copy link
Contributor

@giltho giltho commented Sep 21, 2022

Description of changes:

Instead of inserting a comment field on the Irep for deinit and potential future annotations, inserts a #kani_comment.
It seems that having a field that doesn't that with a # might be dangerous otherwise.

Resolved issues:

Resolves #1575

Call-outs:

Kani still outputs some comment fields, as it used to before #1469, but these comment fields are inside a #c_location field, so they will be ignored by transitivity.

Testing:

This is just a slight change, it should not change anything in kani.

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@giltho giltho requested a review from a team as a code owner September 21, 2022 18:30
@giltho giltho changed the title use with_kani_comment for deinit Use #kani_comment instead of #comment for deinit Sep 21, 2022
/// inside the location sub of the irep.
pub fn with_comment<T: Into<InternedString>>(self, c: T) -> Self {
self.with_named_sub(IrepId::Comment, Irep::just_string_id(c))
pub fn with_kani_comment<T: Into<InternedString>>(self, c: T) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

This ends up with "kani" stuff inside cprover_bindings. Is there a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
The only other way is to use a IrepId::FreeformString.

@tedinski
Copy link
Contributor

Was "comment" ever used? I wonder if we could just change that to #comment and call this done...

@giltho
Copy link
Contributor Author

giltho commented Sep 28, 2022

I think checking with the CBMC team that there wouldn't be a conflict would be enough yes

@celinval
Copy link
Contributor

@giltho @tedinski What's left to be done here?

@giltho
Copy link
Contributor Author

giltho commented Nov 16, 2022

Check with the cbmc team that it's ok to just use #comment and if so, simply do that

@tautschnig
Copy link
Member

There is nothing special about '#comment' in CBMC. It's the '#' prefix itself that is special, but beyond that there are no conflicts for this.

@giltho
Copy link
Contributor Author

giltho commented Dec 5, 2022

Ah, sorry I hadn't seen Michael's answer.
Actually... comments are used to propagate the assert messages to CBMC, and then are given in the CBMC output which is then used by the CBMC_output_parser.
Using #comment instead of comment for IrepId::Comment.to_string() entirely breaks the regression test suite.

@giltho
Copy link
Contributor Author

giltho commented Dec 5, 2022

I renamed KaniComment into HiddenComment, so now

IrepId::HiddenComment => "#comment",
IremId::Comment => "comment"

it's not much better but it does remove mentions of Kani from the cprover_bindings

@tedinski
Copy link
Contributor

tedinski commented Dec 5, 2022

Maybe it's just fine as-is then?

@giltho
Copy link
Contributor Author

giltho commented Dec 6, 2022

Yep, any change was breaking the CI anyway 🤔

@celinval
Copy link
Contributor

celinval commented Dec 8, 2022

So I'm closing this based on the comments. Please feel free to reopen if you think it is still relevant (and I'm sorry if I closed it prematurely). :)

@celinval celinval closed this Dec 8, 2022
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.

Clean GotoC AST annotations

5 participants