[PATCH] D38658: [cfi-verify] Add an interesting unit test where undef search length changes result.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:654
+  uint64_t PrevSearchLengthForUndef = SearchLengthForUndef;
+  SearchLengthForUndef = 5;
   EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 9));
----------------
hctim wrote:
> pcc wrote:
> > Maybe `SearchLengthForUndef` should be a field on `FileAnalysis`?
> My thoughts are that `SearchLengthForUndef` should be a singleton across all instances of `FileAnalysis`. It just so happens that `FileAnalysis` is always a singleton (apart from these unit tests), and I'm not sure that duplicating this data is neccessary. WDYT?
Up to you, but it seems that making it a field would make your tests less error-prone, which to me means that it's probably the "right" architecture.


https://reviews.llvm.org/D38658