Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
370 ms | linux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp |
Event Timeline
Maybe the popular RegisterRef can be moved to Register.h? Would that complicate this patch?
llvm/lib/Target/Hexagon/BitTracker.h | ||
---|---|---|
134 | are Reg and Pos const? | |
146 | Are Reg and Sub const? marking them so would help readability, and also ensure we don't accidentally yank the initializer in the ctor. | |
llvm/lib/Target/Hexagon/HexagonBlockRanges.h | ||
34 | weird... there was one of these earlier. Not sure if hoisting it in a common .h may make sense, but otherwise the comments are the same (const-ing Reg and Sub) | |
llvm/lib/Target/Hexagon/HexagonConstPropagation.cpp | ||
86–87 | seems like a popular design | |
llvm/lib/Target/Hexagon/HexagonExpandCondsets.cpp | ||
190–191 | 4th RegisterRef. Sub is probably unsigned? | |
llvm/lib/Target/Hexagon/HexagonGenPredicate.cpp | ||
51–52 | 5th RegisterRef :) |
I'd definitely prefer not cleaning up RegisterRef as part of this patch. The register/sub tuple pattern is present throughout the code and needs an independant clean-up. Otherwise we risk this not being a NFC.
Regarding const usage, this will delete the implicit copy constructor. Given the earlier statement that those classes need an independant clean-up, are you okay punting on this?
llvm/lib/Target/Hexagon/BitTracker.h | ||
---|---|---|
134 | See comments. | |
llvm/lib/Target/Hexagon/HexagonBlockRanges.h | ||
34 | Yeah this was very odd and requires clean-up | |
llvm/lib/Target/Hexagon/HexagonConstPropagation.cpp | ||
86–87 | See comments. |
Yes, but could you add a FIXME above each, with the same text - so we can fish them out easily afterwards.
are Reg and Pos const?