- User Since
- Jul 22 2016, 3:32 PM (235 w, 5 d)
Nov 7 2020
Nov 6 2020
Nov 5 2020
besides a small nit this looks reasonable to me, I'd wait for @smeenai to take a look as well
Nov 4 2020
Nov 3 2020
Oct 29 2020
Oct 28 2020
@parras - thanks for the patch! According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages to correctly specify the author of the changes we need name + e-mail
Oct 27 2020
Oct 23 2020
I'd wait for @smeenai to take a look as well.
Oct 22 2020
Update the message inside assert(...)
Oct 21 2020
My 0.02$: (perhaps, this should have been mentioned earlier) the current class CopyConfig contains e.g. file names (again, imo it is good enough for a tool, but not good enough for a library) and this means that if somebody wants to add a section to an object file he won't be able to accomplish this task using the current interface without creating extra files. It kind of defeats the idea. To solve this problem proper abstractions should be introduced / the code needs to be refactored. Personally I would strongly prefer to see the following iterative approach here: refactor the current code in llvm-objcopy step by step until it's ready to be moved into a library with a clean and easy-to-use interface. Maybe I'm missing something, but doing refactoring post factum seems to be a less controllable process and might get us to the state where the code has been move out of the tool, the interface has been modified to accomplish a very specific task and the rest (burden) will stay there for years creating more issues than benefits, moreover, it would introduce some risks.
Regarding where to place these functions - into libObject or create a separate library - libObject already contains several write* functions, (e.g. for archives), so indeed, putting this group of functions (e.g. one can use a bit less verbose name - copy(...)) into libObject seems to be quite natural.
Oct 20 2020
Oct 19 2020
According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages your name + e-mail are necessary (to correctly specify the author of the changes).
I'd wait for a day to see if there are any comments from other reviewers, if everything is okay I'll commit it.
Oct 18 2020
Oct 16 2020
Add more tests, 32-bit/64-bit
Oct 13 2020
@thieta - thanks for the patch and for your patience. According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages we would need your name + e-mail (to correctly specify the author of the changes).
Oct 12 2020
Oct 8 2020
I have some general comments / concerns (in addition to the inline comment).
The interface of the library is important and once it's committed and people start using the library in multiple places it might be harder to make changes / fix issues
(unfortunately this has already happened in LLVM a few times in the past) .
Oct 7 2020
just want to add a few more words to explain what I meant: on the one hand this would provide more flexibility on the other hand this would introduce one more option + increase the code size.
If this flexibility is necessary (which is not clear) - yeah, we can add it.
@rupprecht - in general, I'm not against using regex, however, as I mentioned above, it seems like it's not a very frequent use case + there are not so many rpaths (this is quite different from matching symbol names or section names).
Adding more options (--regex) + the required plumbing + tests coverage has its price, honestly I'm not convinced that it's strictly necessary in this particular case (when e.g. a binary has 2 or 3 rpaths).
sorry about the delay, I'll try to review this diff this week / early next week
Oct 6 2020
I don't have a strong opinion regarding this either, usually there are not so many rpaths.
To me the current version looks reasonable, if you'd like I think we can proceed, but it's up to you.
This change needs some analysis & review, it might take some time (especially because this week we have the LLVM conference).
Oct 5 2020
Oct 4 2020
Thanks, looks good, I'd wait for ~1 day to see if other reviewers have any comments, if everything is fine I can commit it for you
Oct 3 2020
Oct 2 2020
(and yeah, we need a test for this option)
Sep 30 2020
Flesh out the comment.
Sep 29 2020
Sep 28 2020
Sep 26 2020
Sep 25 2020
Sep 24 2020
apart from the minor comments, I think this change is in the right direction.
Sep 18 2020
Sep 17 2020