This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Make `runDataflowReturnError()` a non-template function.
ClosedPublic

Authored by mboehme on Jul 3 2023, 6:05 AM.

Details

Summary

It turns out this didn't need to be a template at all.

Likewise, change callers to they're non-template functions.

Also, correct / clarify some comments in RecordOps.h.

This is in response to post-commit comments on https://reviews.llvm.org/D153006.

Diff Detail

Event Timeline

mboehme created this revision.Jul 3 2023, 6:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
mboehme requested review of this revision.Jul 3 2023, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2023, 6:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr2 accepted this revision.Jul 3 2023, 12:51 PM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
389–397

I don't quite understand your comment from the previous patch:

This is really just moved from TransferTest.cpp -- and it's more closely related to the runDataflow() functions there and in the newly added RecordOps.cpp. (I can't call it runDataflow() because then it would differ only in return type from one of the functions in the overload set.)

I don't see another checkDataflow() function with the same signature. Furthermore, checkDataflow() overloads above already return an llvm::Error.

This revision is now accepted and ready to land.Jul 3 2023, 12:51 PM
mboehme added inline comments.Jul 4 2023, 4:14 AM
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
389–397

What I meant is: This function seemed to me to be more closely related to the runDataflow() functions in TransferTest.cpp than to the checkDataflow() functions above -- because of the parameter types (the checkDataflow() functions take AnalysisInputs, while this this function doesn't), and also because this function, like the runDataflow() functions, is not a template, while the checkDataflow() functions are.

Should I ignore these superficial similarities?

gribozavr2 added inline comments.Jul 4 2023, 4:31 AM
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
389–397

Ah I think I understand. I'm just trying to make sense of the many overloads we have here.

Okay so since this function hardcodes NoopAnalysis (unlike checkDataflow()) do you think that should be reflected in the name somehow?

mboehme added inline comments.Jul 4 2023, 4:42 AM
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
389–397

Ah I think I understand. I'm just trying to make sense of the many overloads we have here.

Yup, it's confusing...

Okay so since this function hardcodes NoopAnalysis (unlike checkDataflow()) do you think that should be reflected in the name somehow?

Good point. Maybe this is a path to a good name. How about something like checkDataflowWithNoopAnalysis()? Still a mouthful, but I think it reflects better what this actually does, and it also explains why it's not a template.

gribozavr2 added inline comments.Jul 4 2023, 4:48 AM
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
389–397

SGTM!

mboehme updated this revision to Diff 537061.Jul 4 2023, 5:15 AM

Rename runDataflowReturnError() as discussed in review.

mboehme marked 3 inline comments as done.Jul 4 2023, 5:16 AM
This revision was landed with ongoing or failed builds.Jul 4 2023, 5:44 AM
This revision was automatically updated to reflect the committed changes.