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

Improved stringification for load commands #447

Merged
merged 4 commits into from
Mar 15, 2022

Conversation

apainintheneck
Copy link
Contributor

This PR is related to issue #72.

I added string representations for all of the LoadCommand classes where the .to_s method would return the only obvious, expected result. In some cases, that means that the .to_s just acts as an alias for another method and in other cases it meant just calling the LCStr.to_s method. Either way I didn't delete any of the old methods so there should be no problems with backwards compatibility.

What Changed

  1. Changes to load_commands.rb
    Added .to_s to all easily stringifiable LoadCommand classes
  2. Changes to macho_file.rb
    Removed now unnecessary chained method calls to LoadCommand classes

lib/macho/macho_file.rb Outdated Show resolved Hide resolved
lib/macho/macho_file.rb Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member

Overall LGTM. I'd prefer we keep the uses in macho_file.rb the way they are, but the #to_s implementations themselves look good!

@apainintheneck
Copy link
Contributor Author

Ah, that makes sense. I just reverted that file to its original state.

@woodruffw
Copy link
Member

woodruffw commented Mar 7, 2022

LGTM. Could you add a few small tests for the new #to_s impls? Putting them in the same test blocks as the methods they wrap is fine.

@apainintheneck
Copy link
Contributor Author

I was able to add to the tests in the test_create_load_commands.rb file for the DyLibCommand and RpathCommand classes. Most of the other LoadCommand classes don't really seem to have tests yet as far as I can see so I don't really know where to add those tests.

When I added an assert to the test_macho.rb file related to the SegmentCommand class it brought up a few more linter errors which I solved by changing the style and excluding one of the cops from the test folder. These linter errors only showed up on the pre-commit checks not when running the tests beforehand which was weird.

@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 14, 2022

Never mind I now see that run-tests doesn't actually run the linter so those warnings were always there. I think that it still might be a good idea to exclude that cop from the tests folder either way.

It spits out the following warning type three times.

test/test_macho.rb:150:11: W: Lint/AmbiguousBlockAssociation: 
Parenthesize the param MachO::Sections::SECTION_ATTRIBUTES.keys.any? { |sa| sect.attribute?(sa) } 
to make sure that the block will be associated with the MachO::Sections::SECTION_ATTRIBUTES.keys.any? method call.
          assert MachO::Sections::SECTION_ATTRIBUTES.keys.any? { |sa| sect.attribute?(sa) }

I'm gonna go over to the Rubocop repo and see what they have to say about it.

Edit: I think I understand what the cop is saying now. They want the asserts to look like this. I've made that change so now that warning shouldn't show up for those three lines in the test_mach.rb file.

assert(MachO::Sections::SECTION_ATTRIBUTES.keys.any? { |sa| sect.attribute?(sa) })

1. Changes to load_commands.rb
	Added .to_s to all easily stringifiable LoadCommand classes
2. Changes to macho_file.rb
	Removed now unnecessary chained method calls to LoadCommand classes
I also fixed some other lines that popped up as pre-commit issues after
adding the one line assert to the test_macho.rb file.

I added an exclude statement to stop one warning which I think is just
related to the test framework and the other was just a matter of
changing the style. Just small details.
@woodruffw
Copy link
Member

LGTM, thanks.

@woodruffw woodruffw merged commit 6664172 into Homebrew:master Mar 15, 2022
@apainintheneck apainintheneck deleted the stringify branch March 31, 2022 01:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants