Page MenuHomePhabricator

[Reproducers] Refactor reproducer info

Authored by JDevlieghere on Jan 16 2019, 3:02 PM.



In the original reproducer design, I expected providers to be more dynamic than they turned out. For example, we don't have any instances where one provider has multiple files. Additionally, I expected there to be less locality between capture and replay, with the provider being defined in one place and the replay code to live in another. Both contributed to the design of the provider info.

This patch refactors the reproducer info to be something static. This means less magic strings and better type checking. The new design still allows for the capture and replay code to live in different places as long as they both have access to the new statically defined info class.

I didn't completely get rid of the index, because it is useful for (1) sanity checking and (2) knowing what files are used by the reproducer.

Diff Detail


Event Timeline

JDevlieghere created this revision.Jan 16 2019, 3:02 PM

This looks much better. LGTM, just make sure to do something with the lower_bound search, as I don't think that's right.

58–59 ↗(On Diff #182159)

return StringRef ?

164 ↗(On Diff #182159)

This might even be a vector of StringRefs, if you're ok with saying that the Provider class is responsible for these strings with sufficient lifetime. In the current setup it kind of already is because there's no telling who will use the returned c strings and when.

218–219 ↗(On Diff #182159)

This doesn't seem right. This will only return false if file is lexicographically after all files in the m_files array. Perhaps you meant return it != m_files.end() && *it == file; ?

This revision was not accepted when it landed; it landed in state Needs Review.Jan 17 2019, 5:09 PM
Closed by commit rL351501: [Reproducers] Refactor reproducer info (authored by JDevlieghere, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
JDevlieghere marked 2 inline comments as done.