Page MenuHomePhabricator

[clang] -foptimization-record-file= should imply -fsave-optimization-record
ClosedPublic

Authored by Quolyk on Nov 9 2017, 2:10 AM.

Diff Detail

Repository
rC Clang

Event Timeline

Quolyk created this revision.Nov 9 2017, 2:10 AM
Quolyk retitled this revision from [ClangDriver] -foptimization-record-file= should imply -fsave-optimization-record to [clangd][WIP] -foptimization-record-file= should imply -fsave-optimization-record.Nov 9 2017, 2:18 AM
Quolyk updated this revision to Diff 122395.Nov 9 2017, 11:40 PM
Quolyk retitled this revision from [clangd][WIP] -foptimization-record-file= should imply -fsave-optimization-record to [clangd] -foptimization-record-file= should imply -fsave-optimization-record.

Added 1 test, I guess it's sufficient

Quolyk updated this revision to Diff 123559.Nov 20 2017, 3:41 AM

Hi! You put "[clangd]" in the title, but I don't believe it's related to clangd but rather just "clang"?

Hi! You put "[clangd]" in the title, but I don't believe it's related to clangd but rather just "clang"?

I thought patch to clang option is supposed to be marked as "clangd".

Quolyk retitled this revision from [clangd] -foptimization-record-file= should imply -fsave-optimization-record to [clang] -foptimization-record-file= should imply -fsave-optimization-record.Nov 20 2017, 6:42 AM

I think you can achieve the same result with less code by checking for the flag's presence higher up, where currently OPT_fsave_optimization_record is handled (Clang.cpp:4329). Something like:

if (Args.hasFlag(options::OPT_fsave_optimization_record,
                 options::OPT_fno_save_optimization_record, false) || 
    Args.hasFlag(options::OPT_foptimization_record_file_EQ, 
                 options::OPT_fno_save_optimization_record, false)) {

The test looks good to me.

This also ensures that if fno_save_optimization_record is specified, you don't overwrite it by setting the file. This is definitely something you'd want to add to your test case.

Quolyk updated this revision to Diff 123914.Nov 22 2017, 5:04 AM
Quolyk edited the summary of this revision. (Show Details)Dec 19 2017, 6:03 AM
Quolyk added reviewers: JDevlieghere, hokein.
JDevlieghere accepted this revision.Dec 19 2017, 7:26 AM

Thanks Dmitry, this LGTM!

PS: Let me know if you don't have commit access and want me to commit it for you.

This revision is now accepted and ready to land.Dec 19 2017, 7:26 AM

Thanks Dmitry, this LGTM!

PS: Let me know if you don't have commit access and want me to commit it for you.

I don't have commit access, please commit. Thanks for code review.

This revision was automatically updated to reflect the committed changes.