sgatev accepted this revision. sgatev added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:57 + } + return std::move(Diags); +} ---------------- Better to remove this and rely on NRVO? https://en.cppreference.com/w/cpp/language/copy_elision ================ Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:49 +template <typename DiagsT> struct DiagnoseState { + DiagnoseState(DiagsT &Diags, const Environment &Env) + : Diags(Diags), Env(Env) {} ---------------- samestep wrote: > sgatev wrote: > > Move this to Diagnosis.h? > I'd prefer to keep it in `MatchSwitch.h` alongside `TransferState`, unless we > plan to, e.g., move that type to `DataflowAnalysis.h`. Fair enough, but generally I see `TransferState` and `DiagnoseState` as very specific to the dataflow analysis framework whereas `MatchSwitchBuilder` seems to be a bit more generic so I'd consider separating them. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:76 +class UncheckedOptionalAccessDiagnosis { +public: ---------------- samestep wrote: > samestep wrote: > > sgatev wrote: > > > samestep wrote: > > > > samestep wrote: > > > > > sgatev wrote: > > > > > > Move this to a new UncheckedOptionalAccessDiagnosis.(h,cpp)? > > > > > OK! I'll do that next. > > > > Actually, @sgatev would that go under `FlowSensitive/Models/` or just > > > > under `FlowSensitive/`? > > > I suggest keeping it under `FlowSensitive/Models/` for now. We can change > > > that at a later point if there's a better alternative. > > Where should I put the `UncheckedOptionalAccessModelOptions` type and > > `ignorableOptional` function that need to be shared between both the model > > and the diagnosis? > Same question for the following other helper functions: > > - `hasOptionalType` > - `isOptionalMemberCallWithName` > - `isOptionalOperatorCallWithName` > > Given how much code is shared between the two, I'm really not sure whether > it's the best idea to move this into a separate file in this patch... I suggest either declaring those in the header and using them from `UncheckedOptionalAccessDiagnosis.cpp` or moving them to a separate file and including them both in the model and in the diagnosis. In general I think it's fair if the diagnosis depends on the model, but not the other way around. It's perfectly fine if we do this refactoring in a follow up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127898/new/ https://reviews.llvm.org/D127898 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
