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.

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

refactor: remove remaining chalkConfig field accesses #329

Merged
merged 17 commits into from
Jun 17, 2024

Conversation

ee7
Copy link
Contributor

@ee7 ee7 commented Jun 11, 2024

Issue

#214

Description

Continue from #235, #236, and #237, removing (nearly) every remaining access of a ChalkConfig field.

With this PR, the remaining field accesses of chalkConfig are the below:

$ git log -1 --format='%h %s'
db26f26 refactor(all): avoid chalkConfig.(mark|report)Templates
$ git grep --break --heading --ignore-case 'chalkConfig\.'
src/run_management.nim
31:  get[T](chalkConfig.`@@attrscope@@`, fqn)
34:  getOpt[T](chalkConfig.`@@attrscope@@`, fqn)

Similar to previous PRs, I don't love this class of change, but we can add back some more compile-time safety around this later. The direction was to remove c4autoconf first.

Testing

Hopefully the existing tests are sufficient.

@ee7 ee7 marked this pull request as ready for review June 11, 2024 19:24
@ee7 ee7 requested a review from viega as a code owner June 11, 2024 19:24
@ee7 ee7 requested a review from miki725 June 11, 2024 19:24
This was referenced Jun 11, 2024
src/config.nim Outdated
# Create the item section if required.
if item notin getContents(keys):
discard attrLookup(keys, [item], 0, vlSecDef)
doAssert attrSet(keys, item & ".use", pack(true), Con4mType(kind: TypeBool)).code == errOk
Copy link
Collaborator

Choose a reason for hiding this comment

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

these sets are all kind of really ugly to follow. can we use util template or something to do the same but to make api easier to use?

Suggested change
doAssert attrSet(keys, item & ".use", pack(true), Con4mType(kind: TypeBool)).code == errOk
con4mAttrSet(keys, item & ".use", pack(true))

internally it can get its type from box.kind so no need to pass Con4mType directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to specify the type so that we call the correct overload:

https://github.com/crashappsec/con4m/blob/dc5e76564d7abb962dc6519fd81f34760091b34d/files/con4m/st.nim#L378-L383

https://github.com/crashappsec/con4m/blob/dc5e76564d7abb962dc6519fd81f34760091b34d/files/con4m/st.nim#L401-L406

But all the calls will be changing here soon, and there aren't many attrSet, so I don't think it's worth adding a util template.

Copy link
Collaborator

Choose a reason for hiding this comment

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

internally the util will still pass the Con4mType, but the caller of the utility will just pass a Box and it can normalize internally as necessary

Copy link
Contributor Author

@ee7 ee7 Jun 17, 2024

Choose a reason for hiding this comment

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

I don't want to abstract too much here, but I've pushed 644ed8f. In particular, it'd be more confusing for me at the call site if passing three arguments causes the four-argument overload to be called internally.

We had some attrSet already, but they only acted on ConfigState (for which there is another overload). I've updated all our attrSet to use con4mAttrSet, and only implemented the necessary overloads.

@ee7 ee7 requested a review from miki725 June 17, 2024 17:10
@ee7 ee7 merged commit bad5618 into main Jun 17, 2024
4 checks passed
@ee7 ee7 deleted the ee7/avoid-c4autoconf-part4 branch June 17, 2024 19:06
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.

2 participants