zixuw added inline comments.
================ Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:510 Symbols.emplace_back(std::move(*Obj)); + PathComponentContext.pop_back(); } ---------------- zixuw wrote: > QuietMisdreavus wrote: > > zixuw wrote: > > > What's the cost/would it worth it to wrap the `emplace_back`s and > > > `pop_back`s of `PathComponentContext` in meaningful APIs like > > > `enterPathComponentContext` and `exitPathComponentContext`? > > > That way the code is more self-explanatory and easier to read. It took me > > > some time and mental resources to figure out why the `pop_back` is placed > > > here. > > What's the use of having the `emplace_back` call inside > > `serializeAPIRecord` but to pop it outside? It seems like it's easier to > > mess up for new kinds of records. > The reason to `emplace_back` the path component inside `serializeAPIRecord` > is that the `pathComponent` field of the `Record` is serialized in there so > you want to include the name of the symbol itself in the path component list. > The `pop_back` is delayed til all other additional serialization for a > specific kind of record, for example `serializeEnumRecord` handles all the > enum cases after processing the enum record itself using > `serializeAPIRecord`. So in order to correctly include the enum name in the > path components of all the enum cases, the enum name has to stay in > `PathComponentContext` until all members are serialized. > > This is exactly the reason why I wanted a clearer API to make it easier to > see. Hmm now that I thought about this, it seems that it would be easier to understand and avoid bugs if we lift `PathComponentContext.emplace_back`/`enterPathComponentContext` out of `serializeAPIRecord`, because we have access to the record name before that anyway. So we establish a pre-condition of `serializeAPIRecord` that the correct path components would be set up in `PathComponentContext` before the call so we could still serialize the field inside that method. And in specific `serialize*Record` methods we push the record name, and pop out at the end. This way the push and pop operations would appear in pairs in a single block, saving the confusion and mental work of jumping across functions to see how `PathComponentContext` is evolving. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123045/new/ https://reviews.llvm.org/D123045 STAMPS actor(@zixuw) application(Differential) author(@dang) herald(H423) herald(H576) herald(H864) herald(H875) herald(H876) monogram(D123045) object-type(DREV) phid(PHID-DREV-d5goxxqici5xa3w2bgjy) reviewer(@QuietMisdreavus) reviewer(@ributzka) reviewer(@zixuw) revision-repository(rG) revision-status(needs-review) subscriber(@cfe-commits) tag(#all) tag(#clang) via(web) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
