This is an archive of the discontinued LLVM Phabricator instance.

[Profile] deprecate __llvm_profile_override_default_filename [part-2]
ClosedPublic

Authored by davidxl on Jul 20 2016, 11:28 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl updated this revision to Diff 64825.Jul 20 2016, 11:28 PM
davidxl retitled this revision from to [Profile] deprecate __llvm_profile_override_default_filename [part-2].
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Jul 21 2016, 2:33 PM

Minor nits.

lib/profile/InstrProfilingFile.c
473 ↗(On Diff #64825)

Nit, might be easier to read as an if-stmt + ternary.

test/profile/instrprof-override-filename-with-env.c
2 ↗(On Diff #64825)

Does this test anything useful now? Can we just delete this file?

test/profile/instrprof-override-filename.c
2 ↗(On Diff #64825)

Ditto, we should just be able to delete this file.

davidxl added inline comments.Jul 21 2016, 3:11 PM
test/profile/instrprof-override-filename-with-env.c
2 ↗(On Diff #64825)

This one still has value to test path precedence.

test/profile/instrprof-override-filename.c
2 ↗(On Diff #64825)

Same here -- it can serve as a basic test that = form of the option override the default.

davidxl updated this revision to Diff 64975.Jul 21 2016, 3:20 PM
davidxl edited edge metadata.

Addressed review feedback

vsk accepted this revision.Jul 21 2016, 3:27 PM
vsk edited edge metadata.

Looks good.

test/profile/instrprof-override-filename-with-env.c
2 ↗(On Diff #64975)

Ok

This revision is now accepted and ready to land.Jul 21 2016, 3:27 PM
This revision was automatically updated to reflect the committed changes.