Add routines to compare measurements against reference #167
Conversation
|
I can't figure out the lint error; I'm unsure how to fix it. |
I suggest giving a go at: goimports -local "github.com/veraison/corim" -w . |
Thanks, @thomas-fossati; that helped to fix it. As Dionna suggested in the meeting, the fix needed a new line between "fmt" and the following line. I'm working on the test coverage and will post a new revision of the changes shortly. Thank you! |
81e77c4 to
23a1fb7
Compare
There was a problem hiding this comment.
As a general note, it's weird for Compare() method to raise an error. Two values not being equal is not automatically considered an error condition. Also, I would expect a method called "Compare" to do a relative comparison, rather than equality test.
I would rename the method to Equal and have it return a bool instead.
(Alternatively, you could keep it as "Compare" and have it return an int, with 1 indicating o > r, -1 indicating r > o, and 0 indicating o == r; however, that would require chainging the implementations as well, and would require for all types in question to have partial ordering, which I don't think we want.)
Do you use vim or VS Code? If so, the popular go plugins (e.g., vim-go) should automatically update the import statements order transparently. |
There was a problem hiding this comment.
I agree with @setrofim high-level comment.
I think we should have:
- a
Comparemethod for types for which ordering makes sense, e.g., SVNs -- and it should return -1,0,1 rather than error; - an
Equalmethod for types for which we are only concerned about strict equality (e.g., digests) -- and this could return a bool.
I think we should have |
WFM |
I also prefer a boolean return value. If we cannot perform the comparison due to an error, should we log it and return false for a result?
It's not always equality; the type governs the semantics of comparison. In the SVN type, for example, the comparison should be equality for the ExactValueType and greater-than-or-equal for MinValueType. |
|
OK, I'll push a PR with recommended changes. Thanks @thomas-fossati & @setrofim for the review. |
1695f74 to
4b374fc
Compare
4b374fc to
03b633e
Compare
|
@thomas-fossati @setrofim could you please confirm if the PR looks good? Thanks! |
There was a problem hiding this comment.
Looks good, thanks @jraman567 !
There's one point to discuss about what to do with RawValue.
03b633e to
77d243f
Compare
|
@thomas-fossati and @setrofim, I have addressed your comments. Additionally, I made the following changes:
Also, the lint error that CI reports (Implicit memory aliasing in for loop), is a false positive. It doesn't apply for golang versions 1.22 and newer. Thank you for your review! |
"Compare" as an operation has fairly well established semantics, which your methods do not implement. For example, in Go standard library: https://pkg.go.dev/strings#Compare In other libraries: https://pkg.go.dev/github.com/open-policy-agent/opa/ast#Compare And in other languages, stretching all the way back to C (in the form of Please also note that the spec specifically talks about "Comparing a condition ECT against the ACS", and not about comparing two values of those types in general. The defined operation is not a generic comparison, and so the method name should not imply that it is. If you wish to stay closer to the spec, perhaps something like Please rename. |
|
Sounds reasonable. I'll change the name to |
- refactor Version type into a separate file - add equality and comparison methods Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
- Add methods to test the equality of TaggedSVN & TaggedMinSVN types - Add comparison methods for TaggedSVN as outlined in the spec Signed-off-by: Sergei Trofimov <sergei.trofimov@arm.com> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
77d243f to
93b1d2b
Compare
comid/digests.go
Outdated
| @@ -41,6 +43,54 @@ func (o Digests) Valid() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (o *Digests) Equal(r *Digests) bool { | |||
There was a problem hiding this comment.
Since we're not modifying o, should this method have an object, rather than pointer, receiver (ditto for all other methods)?
At one point, we had the convetion that for methods that do not modify the internal state should have an object receiver to indicate that.
There was a problem hiding this comment.
My personal opinion is to avoid mixing receiver types; we should pick a pointer or value type and stay consistent for the whole object. If an object is immutable, it should have a value type; it can take a pointer type otherwise.
I’m obviously unaware of your convention. Please confirm if you want to stick to a single type for an object or change the receiver type depending on whether the method modifies the object. Thanks.
There was a problem hiding this comment.
I don't have a strong opinion on this. By using pointer receivers you avoid copying ponetially large values (here, Digests is already a slice, which is tiny, so we don't win much in this case); by using a value receiver you do not risk modifying state from a method that is not intended to do so, and clearly communicate that (lack of) intent to the reader.
I don't see the advantage of avoiding switching receiver type between methods, depending on what the method needs. Consitently using pointer receiver in methods that don't need them just because other methods modify state helpes neither readability nor usability.
However, I don't see this as a major issue. So if the others are ok with it, I'm fine with code going in as-is.
There was a problem hiding this comment.
I don't have a strong opinion on this. By using pointer receivers you avoid copying ponetially large values (here,
Digestsis already a slice, which is tiny, so we don't win much in this case); by using a value receiver you do not risk modifying state from a method that is not intended to do so, and clearly communicate that (lack of) intent to the reader.I don't see the advantage of avoiding switching receiver type between methods, depending on what the method needs. Consitently using pointer receiver in methods that don't need them just because other methods modify state helpes neither readability nor usability.
However, I don't see this as a major issue. So if the others are ok with it, I'm fine with code going in as-is.
I don't see a strong argument for changing the "good ole" convention 😃
There was a problem hiding this comment.
I switched the receiver types to the one @setrofim proposed. However, the linter complained that the receiver for FlagsMap was too big, so I reverted it to a pointer.
I did the same for Digests & IntegrityRegisters, which have the potential to get big. All others follow the existing convention.
There was a problem hiding this comment.
I am still pondering whether it's better (or worse) to have an inconsistent situation (both pointer & value receiver for "read-only" methods) or one that allows unexpected modifications of the underlying object (i.e., pointer receiver in all cases).
There was a problem hiding this comment.
My 2 cents:
- The advantage of allowing pointer receivers for read-only is real, if
negligible. You avoid copying large objects (FlagsMap-- as
@thomas-fossati noted,Digests(andIntegrityRegistersalso) is a slice,
so we'd only be copying an additional two ints in those cases). - The advantages of forcing value receivers for read-only are speculative.
Accidental modification of state would be caught through tests or code
review. And it's unclear what the marginal use is of the convention in
documenting a method's behaviour (it's read-only) beyond the method's name
and just looking thorough its implementation.
We have 1 concrete but negligible points for pointer receivers vs two
speculative points for value receivers on read-only methods.
I don't think there is a clear winner on merit here.
The other thing to consider is consistency with the rest of the code base. I
believe, until now, we have stuck with the convention that read-only methods
have value receivers. Linter complaints can be suppressed per-line using
//nolint as needed.
But like I said above, personally I don't have a strong preference either way.
</bike shedding>
There was a problem hiding this comment.
Sorry, @thomas-fossati . I missed your comment from yesterday.
Due to how Golang passes slices around, could we even have read-only methods for slices since the methods could modify them partially (they have access to the underlying array)?
There was a problem hiding this comment.
It would be read-only with respect to the slice (i.e. you would not be able to, e.g. append elements), but not with respect to its contents.
"read-only" also may not be, strictly speaking, correct here. You're still able to modify the receiver variable, even with a value receiver. However, since it is a copy, the modifications will not be visible outside the method.
There was a problem hiding this comment.
The other thing to consider is consistency with the rest of the code base. I
believe, until now, we have stuck with the convention that read-only methods
have value receivers. Linter complaints can be suppressed per-line using
//nolint as needed.
👍
comid/digests.go
Outdated
| @@ -41,6 +43,54 @@ func (o Digests) Valid() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (o *Digests) Equal(r *Digests) bool { | |||
There was a problem hiding this comment.
I don't have a strong opinion on this. By using pointer receivers you avoid copying ponetially large values (here, Digests is already a slice, which is tiny, so we don't win much in this case); by using a value receiver you do not risk modifying state from a method that is not intended to do so, and clearly communicate that (lack of) intent to the reader.
I don't see the advantage of avoiding switching receiver type between methods, depending on what the method needs. Consitently using pointer receiver in methods that don't need them just because other methods modify state helpes neither readability nor usability.
However, I don't see this as a major issue. So if the others are ok with it, I'm fine with code going in as-is.
comid/digests.go
Outdated
| @@ -41,6 +43,54 @@ func (o Digests) Valid() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (o *Digests) Equal(r *Digests) bool { | |||
There was a problem hiding this comment.
I don't have a strong opinion on this. By using pointer receivers you avoid copying ponetially large values (here,
Digestsis already a slice, which is tiny, so we don't win much in this case); by using a value receiver you do not risk modifying state from a method that is not intended to do so, and clearly communicate that (lack of) intent to the reader.I don't see the advantage of avoiding switching receiver type between methods, depending on what the method needs. Consitently using pointer receiver in methods that don't need them just because other methods modify state helpes neither readability nor usability.
However, I don't see this as a major issue. So if the others are ok with it, I'm fine with code going in as-is.
I don't see a strong argument for changing the "good ole" convention 😃
93b1d2b to
09a73cd
Compare
|
@thomas-fossati @setrofim Thanks for your review! I addressed all your comments, except for the receiver types for |
- add a method to test if two digests are equal - add a method to compare a digests object against reference, as outlined in the CoRIM spec Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Add equality and comparison methods for FlagsMap object Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
- add a method to test if a RawValue type equals another one - add a comparison method to check against a reference Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Add equality and comparison methods to test against reference Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
- Add a method to test if an IntegrityRegisters object equals another - Add a method to compare IntegrityRegisters object against a reference per CoRIM spec Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
09a73cd to
3736981
Compare
| @@ -41,6 +42,86 @@ func (o Digests) Valid() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Equal confirms if the Digests object is the same as another | |||
There was a problem hiding this comment.
| // Equal confirms if the Digests object is the same as another | |
| // Equal confirms if the Digests object is equal |
There was a problem hiding this comment.
Sorry, @yogeshbdeshpande . I didn't see your comments. Let me submit another PR with these changes.
| return false | ||
| } | ||
|
|
||
| ref := refDigests |
There was a problem hiding this comment.
I got a linter error if I used refDigests directly. So, I created a copy of it.
| func (o RawValue) Equal(r RawValue) bool { | ||
| return reflect.DeepEqual(o, r) | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you please add a header comment to this method and reference the comparison section!
| return ret == 0 | ||
| } | ||
|
|
||
| // Compare helper function to compare two SVNs, object and reference |
There was a problem hiding this comment.
| // Compare helper function to compare two SVNs, object and reference | |
| // compare is a helper function to compare two SVNs, object and reference |
There was a problem hiding this comment.
OK, will change it
|
Thanks, @thomas-fossati & @setrofim for reviewing! |
|
Sorry, @yogeshbdeshpande . I didn't see your comment before submitting; I guess I opened the PR page when Thomas approved it, and I was in a meeting at that time. I merged it when the meeting got over, without refreshing the page. |
Adds a compare routine to some of the measurement types.