Page MenuHomePhabricator

[llvm][TextAPI] add equality operator for InterfaceFile

Authored by spowell on Feb 12 2021, 1:13 PM.



This patch adds functionality to compare for the equality between InterfaceFiles based on attributes specific to linking.

Diff Detail

Event Timeline

spowell created this revision.Feb 12 2021, 1:13 PM
spowell requested review of this revision.Feb 12 2021, 1:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 1:13 PM
cishida added inline comments.Feb 16 2021, 11:51 AM

can we use llvm::lower_bound instead? it supports passing a range instead of boilerplate iterators.

spowell updated this revision to Diff 324100.Feb 16 2021, 1:38 PM

updated std::lower_bound to llvm::lower_bound

Good in general. Suggestions in line.


you don't need {} for one line of body.


Again, omit {}


Rather than spell out all the transform and reset back to equal, I will just create a wrapper that takes a function that transform one of the input, make sure they are different, transform the other, make sure they are equal now. It would be a lot cleaner and easy to read.

Same as below which you recreates the TBDFile from inputs again and again, I will create a wrapper to avoid duplicated code. Same as TBDv4 tests.

spowell updated this revision to Diff 324400.Feb 17 2021, 1:01 PM

refactored tests to implement wrapper, removed single line {}, changed Document.insert to Document.emplace

cishida added inline comments.Feb 17 2021, 1:10 PM

These functions are the same except variable name between these files, we have a TextStubHelpers.h file in the unit tests, can we just move these there?


If this is refactoring the multiple times we use this, it should just be a variable we can call not a function.


how about updateAttributeThenReset?

cishida added inline comments.Feb 17 2021, 1:24 PM

Can we avoid re-reading in the same input string to reset these variables and instead have one variable in the test that doesn't get altered and pass that to reassign the altered ones.


sorry, I mean a const variable we can just refer to.

steven_wu accepted this revision.Feb 17 2021, 1:46 PM

Looks good. with a small comment inline.


NIT: You can redo this function as a lambda in the test function so it only takes the transform function as parameter. FileA and FileB is freshly created from TBD file in the beginning and the TBDv3File can just be a lambda capture.

This revision is now accepted and ready to land.Feb 17 2021, 1:46 PM
spowell updated this revision to Diff 324684.Feb 18 2021, 9:57 AM

moved transform wrapper to TextStubHelpers, refactored inequality tests for both v3 and v4.

steven_wu accepted this revision.Feb 18 2021, 10:52 AM
This revision was automatically updated to reflect the committed changes.