This will match the locations attached to the IRunits passed in as context
with an action.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Debug/BreakpointManagers/FileLineColLocBreakpointManager.h | ||
---|---|---|
63 | does this work on PointerUnion? | |
64 | drop trivial braces | |
99 | Couldn't you hash the FileLineColLoc directly? | |
mlir/lib/Debug/BreakpointManagers/FileLineColLocBreakpointManager.cpp | ||
56 | e | |
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp | ||
106 | would it make more sense to set up this machinery inside MlirOptMain.cpp based on the string option? The static tricks going on here feel a little automagical | |
mlir/unittests/Debug/FileLineColLocBreakpointManagerTest.cpp | ||
233 | newline |
mlir/include/mlir/Debug/BreakpointManagers/FileLineColLocBreakpointManager.h | ||
---|---|---|
63 | It does not | |
99 | Absolutely! | |
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp | ||
106 |
We are inside MlirOptMain.cpp :) I could move the vector outside of the lambda if you prefer, but it would still be static (like all the cl:: options actually. I don't quite see where to handle this differently actually? The config does not hold a vector of strings and none of the cl options are available anywhere else. |
mlir/include/mlir/Debug/BreakpointManagers/FileLineColLocBreakpointManager.h | ||
---|---|---|
23 | Can you document this class? | |
53–54 | Same here, would love docs on this. | |
56 | Can you add documentation to the various methods in this class? | |
72 | Drop the trivial braces here. | |
88–92 | Can we make this private? | |
89 | Can we use StringAttr instead of StringRef here? Will be much more optimal on lookup. | |
94 | Please use std::optional in new code. | |
95–97 | Can we have a little bit more advanced checking here? This will miss out on things like FusedLoc/NameLoc/etc. | |
mlir/include/mlir/Debug/Observers/ActionLogging.h | ||
33–34 | Can you add a little more context here? I had to read the comment a few times to understand what it meant. | |
mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h | ||
89–90 | This comment feels slightly unrelated to the function, e.g. doesn't mention break point manager at all. Can this be tweaked? | |
95–96 | I'm missing the connection between LocFilters(in the function name) and location breakpoints to use for logging actions? Can this comment be tweaked/beefed up? | |
mlir/lib/Debug/BreakpointManagers/FileLineColLocBreakpointManager.cpp | ||
18 | Likely can drop the llvm:: here. | |
45–50 | Can you update the diagnostic function to use Twine instead of StringRef? That would simplify this function. |
Address comments
mlir/include/mlir/Debug/BreakpointManagers/FileLineColLocBreakpointManager.h | ||
---|---|---|
56 | These are override, do we usually repeat the parent comment here? | |
89 | I don't think so: we don't have an MLIRContext when we create breakpoints. Imagine a gdb session connected to a process with multiple MLIRContext and you set a breakpoint based on a filename. | |
95–97 | NameLoc isn't relevant I believe, FusedLoc matters though, using a walk should do? |
mlir/include/mlir/Debug/BreakpointManagers/FileLineColLocBreakpointManager.h | ||
---|---|---|
56 | Missed it was override. Generally we don't if the behavior is clearly defined from the class, which should be the case here (if we get a class doc). | |
89 | Gotcha, makes sense. | |
95–97 | Yeah, though I think you'd just have to be careful to not include caller locations from callsites. |
@rriddle : I think I addressed all of your comments? (judging by your last answer), but you didn't approve so I'm not sure if you wanted to review more?
Can you document this class?