samestep added inline comments.
================ Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:57 + } + return std::move(Diags); +} ---------------- sgatev wrote: > Better to remove this and rely on NRVO? > https://en.cppreference.com/w/cpp/language/copy_elision Ah OK, I'm not a C++ expert so I wasn't sure when to write `std::move` and when not to. Will do, thanks! ================ 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) {} ---------------- sgatev wrote: > 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. Sure, if people think it makes sense to put those two types elsewhere then we can do that in a followup. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:76 +class UncheckedOptionalAccessDiagnosis { +public: ---------------- sgatev wrote: > 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. Yep, makes sense; I just didn't want to make declarations for all those in a header file in this patch. 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
