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.

Sign up for GitHub

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

Merge "extra" lint target into "normal" lint target #153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+147 −133
Open

Merge "extra" lint target into "normal" lint target #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/workflows/linters.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,3 @@ jobs:
go install github.com/golang/mock/[email protected]
- name: Run required linters in .golangci.yml plus hard-coded ones here
run: make lint
- name: Run optional linters (not required to pass)
run: make lint-extra
17 changes: 5 additions & 12 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,10 @@ GOPKG += github.com/veraison/corim/extensions

GOLINT ?= golangci-lint

ifeq ($(MAKECMDGOALS),lint)
GOLINT_ARGS ?= run --timeout=3m
else
ifeq ($(MAKECMDGOALS),lint-extra)
GOLINT_ARGS ?= run --timeout=3m --issues-exit-code=0 -E dupl -E gocritic -E gosimple -E lll -E prealloc
endif
endif
GOLINT_ARGS ?= run --timeout=3m -E dupl -E gocritic -E gosimple -E lll -E prealloc

.PHONY: lint lint-extra
lint lint-extra:
.PHONY: lint
lint:
$(GOLINT) $(GOLINT_ARGS)

ifeq ($(MAKECMDGOALS),test)
Expand Down Expand Up @@ -50,7 +44,7 @@ presubmit:
@echo
@echo ">>> Fix any lint error"
@echo
$(MAKE) lint-extra
$(MAKE) lint

.PHONY: licenses
licenses: ; @./scripts/licenses.sh
Expand All @@ -60,8 +54,7 @@ help:
@echo "Available targets:"
@echo " * test: run unit tests for $(GOPKG)"
@echo " * test-cover: run unit tests and measure coverage for $(GOPKG)"
@echo " * lint: lint sources using default configuration"
@echo " * lint-extra: lint sources using default configuration and some extra checkers"
@echo " * lint: lint sources using default configuration and some extra checkers"
@echo " * presubmit: check you are ready to push your local branch to remote"
@echo " * help: print this menu"
@echo " * licenses: check licenses of dependent packages"
2 changes: 2 additions & 0 deletions comid/classid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ func (o *ClassID) UnmarshalCBOR(data []byte) error {
// uuid: standard UUID string representation, e.g. "550e8400-e29b-41d4-a716-446655440000"
// int: an integer value, e.g. 7
// bytes: a variable length opaque bytes, example {0x07, 0x12, 0x34}

//nolint:dupl
func (o *ClassID) UnmarshalJSON(data []byte) error {
var tnv encoding.TypeAndValue

Expand Down
4 changes: 4 additions & 0 deletions comid/comid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ func (o *Comid) AddDevIdentityKey(val KeyTriple) *Comid {
return o
}

// nolint:gocritic
func (o Comid) Valid() error {
if err := o.TagIdentity.Valid(); err != nil {
return fmt.Errorf("tag-identity validation failed: %w", err)
Expand All @@ -267,6 +268,7 @@ func (o Comid) Valid() error {
}

// ToCBOR serializes the target Comid to CBOR
// nolint:gocritic
func (o Comid) ToCBOR() ([]byte, error) {
if err := o.Valid(); err != nil {
return nil, err
Expand All @@ -291,6 +293,7 @@ func (o *Comid) FromCBOR(data []byte) error {
}

// ToJSON serializes the target Comid to JSON
// nolint:gocritic
func (o Comid) ToJSON() ([]byte, error) {
if err := o.Valid(); err != nil {
return nil, err
Expand All @@ -314,6 +317,7 @@ func (o *Comid) FromJSON(data []byte) error {
return encoding.PopulateStructFromJSON(data, o)
}

// nolint:gocritic
func (o Comid) ToJSONPretty(indent string) ([]byte, error) {
if err := o.Valid(); err != nil {
return nil, err
Expand Down
6 changes: 3 additions & 3 deletions comid/example_cca_realm_refval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ func extractRealmRefVal(rv ValueTriple) error {
}
return nil
}

func extractMeasurements(m Measurements) error {
if len(m.Values) == 0 {
return fmt.Errorf("no measurements")
}
for i, meas := range m.Values {
for i := range m.Values {
meas := &m.Values[i]
if err := extractMeasurement(meas); err != nil {
return fmt.Errorf("extracting measurement at index %d: %w", i, err)
}
Expand All @@ -91,7 +91,7 @@ func extractMeasurements(m Measurements) error {
return nil
}

func extractMeasurement(m Measurement) error {
func extractMeasurement(m *Measurement) error {
if err := extractRealmPersonalizationValue(m.Val.RawValue); err != nil {
return fmt.Errorf("extracting realm personalization value: %w", err)
}
Expand Down
7 changes: 4 additions & 3 deletions comid/example_cca_refval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ func extractCCARefVal(rv ValueTriple) error {
return fmt.Errorf("extracting impl-id: %w", err)
}

for i, m := range rv.Measurements.Values {
for i := range rv.Measurements.Values {
m := &rv.Measurements.Values[i]
if m.Key == nil {
return fmt.Errorf("missing mKey at index %d", i)
}
Expand All @@ -75,10 +76,10 @@ func extractCCARefVal(rv ValueTriple) error {
return fmt.Errorf("extracting cca-refval-id: %w", err)
}
if err := extractRawValue(m.Val.RawValue); err != nil {
return fmt.Errorf("extracting raw vlue: %w", err)
return fmt.Errorf("extracting raw value: %w", err)
}
default:
return fmt.Errorf("unexpected Mkey type: %T", t)
return fmt.Errorf("unexpected Mkey type: %T", t)
}
}

Expand Down
7 changes: 4 additions & 3 deletions comid/example_psa_refval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,16 @@ func extractSwMeasurements(m Measurements) error {
if len(m.Values) == 0 {
return fmt.Errorf("no measurements")
}
for i, m := range m.Values {
if err := extractSwMeasurement(m); err != nil {
for i := range m.Values {
meas := &m.Values[i]
Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

traditionally golang always follows conciseness!

Wondering what lint warning is thrown for this change, I mean is line 73 is this change really needed ..?

Could you please clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

The old L72 makes a copy of m at each iteration.
The new L72 takes a reference instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then should not it be better to set extractSwMeasurement(&m)
Or if copy needs to be avoided, then

extractSwMeasurement(&meas.Values[i])

Copy link
Contributor

Choose a reason for hiding this comment

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

Then should not it be better to set extractSwMeasurement(&m)

hmm no, that changes nothing wrt the copy. It's already happened

Or if copy needs to be avoided, then

extractSwMeasurement(&meas.Values[i])

I guess you mean extractSwMeasurement(&m.Values[i])?
If so, it'd be equivalent to the proposed change, except it's a bit less readable.

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande Jan 14, 2025 •
edited by thomas-fossati
Loading

Choose a reason for hiding this comment

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

Then should not it be better to set extractSwMeasurement(&m)

hmm no, that changes nothing wrt the copy. It's already happened

True that , however now we copy a pointer instead of a value, how different then it is to the original change ?

Or if copy needs to be avoided, then
extractSwMeasurement(&meas.Values[i])

I guess you mean extractSwMeasurement(&m.Values[i])? If so, it'd be equivalent to the proposed change, except it's a bit less readable.

ok, but this avoids one extra variable addition, so much neater, in my view!

Copy link
Contributor

Choose a reason for hiding this comment

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

Then should not it be better to set extractSwMeasurement(&m)

hmm no, that changes nothing wrt the copy. It's already happened

True that , however now we copy a pointer instead of a value, how different then it is to the original change ?

In yours, the copy has already happened in the for statement, you are not avoiding it by taking the reference to m: it's too late.
In the proposed change, the measurement object is not copied.

Or if copy needs to be avoided, then
extractSwMeasurement(&meas.Values[i])

I guess you mean extractSwMeasurement(&m.Values[i])? If so, it'd be equivalent to the proposed change, except it's a bit less readable.

ok, but this avoids one extra variable addition, so much neater, in my view!

Readability is, by definition, in the eye of the reader :-)
Personally, I do not like cramming too much information into a single line, therefore I prefer the proposed change.
Anyway, this is not a hill I'd die on :-) I'll let @Akhilesh-max decide.

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, i did not like the suggested change from lint-extra, it kind of defeats the purpose of how effectively one uses golang for loop, to have access to its index and its variables in a single sentence.

if err := extractSwMeasurement(meas); err != nil {
return fmt.Errorf("extracting measurement at index %d: %w", i, err)
}
}
return nil
}

func extractSwMeasurement(m Measurement) error {
func extractSwMeasurement(m *Measurement) error {
if err := extractPSARefValID(m.Key); err != nil {
return fmt.Errorf("extracting PSA refval id: %w", err)
}
Expand Down
Loading
Loading