kadircet added a comment.
mostly LG, i haven't looked at the tests yet though.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1344
+ auto Result = declToHierarchyItem<TypeHierarchyItem>(ND);
+ if (Result) {
+ Result->deprecated = ND.isDeprecated();
----------------
nit: redundant braces
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1353
+ auto Result = declToHierarchyItem<CallHierarchyItem>(ND);
+ if (Result) {
+ if (ND.isDeprecated())
----------------
nit: merge the two conditions and drop the braces.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1361
+template <typename HierarchyItem>
+static Optional<HierarchyItem> symbolToHierarchyItem(const Symbol &S,
+ PathRef TUPath) {
----------------
can you qualify `Optional` here and elsewhere?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1381
- return std::move(THI);
+ return std::move(HI);
+}
----------------
nit: drop `std::move`
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1387
+ auto Result = symbolToHierarchyItem<TypeHierarchyItem>(S, TUPath);
+ if (Result) {
+ Result->deprecated = (S.Flags & Symbol::Deprecated);
----------------
nit: redundant braces
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1396
+ auto Result = symbolToHierarchyItem<CallHierarchyItem>(S, TUPath);
+ if (Result) {
+ if (S.Flags & Symbol::Deprecated) {
----------------
nit: again merge conditions and drop braces
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1620
+ if (auto CHI = declToCallHierarchyItem(*Decl))
+ Result.push_back(*std::move(CHI));
+ }
----------------
`Results.emplace_back(std::move(*CHI))` ?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1629
+ return llvm::None;
+ Expected<SymbolID> ID = SymbolID::fromStr(Item.data);
+ if (!ID) {
----------------
nit: `auto ID`
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1646
+ Request.Filter = RefKind::Reference;
+ // Initially store the results in a map keyed by SymbolID.
+ // This allows us to group different calls with the same caller
----------------
... by SymbolID of the caller.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1649
+ // into the same CallHierarchyIncomingCall.
+ llvm::DenseMap<SymbolID, CallHierarchyIncomingCall> ResultMap;
+ // We can populate the keys of ResultMap, and the FromRanges fields of
----------------
nit: `s/ResultMap/CallsIn` ?
I would also suggest changing value type to `SmallVector<Range, 1>` that way
rather than having a "partially filled" IncomingCall objects in the middle, you
can create valid ones directly within the lookup.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1660
+ }
+ auto It = ResultMap.find(R.Container);
+ if (It == ResultMap.end())
----------------
nit: `auto It = ResultMap.try_emplace(R.Container,
CallHierarchyIncomingCall{}).first`
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1671
+ auto It = ResultMap.find(Caller.ID);
+ if (It != ResultMap.end()) {
+ if (auto CHI = symbolToCallHierarchyItem(Caller, Item.uri.file()))
----------------
why don't we assert this instead?
================
Comment at: clang-tools-extra/clangd/XRefs.h:109
+/// Get call hierarchy information at \p Pos.
+llvm::Optional<std::vector<CallHierarchyItem>>
+prepareCallHierarchy(ParsedAST &AST, Position Pos, const SymbolIndex *Index,
----------------
nridge wrote:
> kadircet wrote:
> > what's the distinction between empty and none return values ? (same for
> > others)
> Generally speaking, a `None` result means "the input was not recognized in
> some way", while an empty vector result means "there are no results for this
> input".
>
> For `prepareCallHierarchy`, the inputs encode a source location, and the
> operation asks "give me `CallHierarchyItem`s for suitable declarations (i.e.
> functions) at this location. So, `None` means "the source location could not
> be recognized" (`sourceLocationInMainFile` failed), while an empty result
> means "there are no declarations of functions at this location".
>
> For `incomingCalls` and `outgoingCalls`, the inputs encode a declaration of a
> function, and the operation asks "give me its callers / callees". So, a
> `None` means "could not locate the function (i.e. symbol)", while an empty
> result means "this function has no callers / callees".
> Generally speaking, a None result means "the input was not recognized in some
> way", while an empty vector result means "there are no results for this
> input".
Makes sense, but that sounds like an implementation detail. I don't see any
difference between the two from caller's point of view. Even the logging
happens inside the implementation. As for interpreting llvm::None by the
caller, i believe it is subject to change, so unless we return an Expected,
there's not much value in returning an Optional, and even in such a case,
caller probably can't do much but propagate the error (maybe also try with
Pos+/-1, but usually that's handled within the XRefs as well).
Returning an empty container is also coherent with rest of the APIs we have in
XRefs.h.
So I believe we should drop the optionals here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91122/new/
https://reviews.llvm.org/D91122
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
