<div dir="ltr">I think it depends on how you interpret the origin. You could tie the origin to the index or to the producer of the symbol. The current model seems to be the later (origin is a configuration in SymbolCollector. MergeIndex manipulates the origin, but I would rather treat that as a special case). And I think it's conceptually simpler and more generic (origin can be more than dynamic vs static). The downside is probably that all symbols in yaml would have the same origin, which is a bit redundant but doesn't seem to matter much. <div><br></div><div>Both models have their pros and cons, but I don't think it's worth spending much time figuring out which one is better when it seems like a non-issue.<br><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 6:58 PM Ilya Biryukov <<a href="mailto:ibiryukov@google.com">ibiryukov@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">Eric Liu <<a href="mailto:ioeric@google.com" target="_blank">ioeric@google.com</a>> schrieb am Di., 25. Sep. 2018, 01:22:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 6:34 AM Ilya Biryukov <<a href="mailto:ibiryukov@google.com" rel="noreferrer" target="_blank">ibiryukov@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Why would we want to serialize the origin? </div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>We only serialize and deserialize for the static index, it does not seem to be useful to serialize origin in that scenario.</div></div></blockquote></div><div dir="ltr"><div class="gmail_quote"><div>We serialize Origin because it's a property of Symbol? The origin for the current YAML symbols is defaulted to "unknown". </div><div></div></div></div></div></blockquote></div></div></div><div dir="auto"><div dir="auto">My view would be that it's a property of a symbol living in memory, but not the serialized one. E.g. all symbols read from the yaml index should have the static origin. If we store an origin, we allow loading symbols with non-static origin, it's hard to see how that would be useful.</div></div><div dir="auto"><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>It's true that we *currently* only serialize symbols from static index, but there is nothing preventing us from doing it for other indexes with different origins. You could probably override origins when loading static index, but that doesn't seem to work in general.</div></div></div></div></blockquote></div></div></div><div dir="auto"><div dir="auto">The origin seems to be exactly the field that is set and manipulated by index implementations, but they all have the knowledge on what their origin is or how to combine origins of subindexes.</div><div dir="auto"><br></div><div dir="auto">Again, it seems there are two classes hiding in symbol. All other fields provide useful information about C++ semantics of the symbol, while origin provides some traceability when combining indexes. The former is something we need to serialize, the latter is something we can infer when deserializing.</div><div dir="auto"><br></div><div dir="auto">If we will have a use case for serializing the latter entity(with origin) for any reason, we might add that separately.</div><div dir="auto"><br></div><div dir="auto">Not sure if it's worth the trouble changing it, just wanted to point out that storing the origin for each symbol in the yaml index for the purpose of indicating that the symbol is in the yaml index seems to be a bit off.</div></div><div dir="auto"><div dir="auto"><br></div><div dir="auto"></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>I checked this in without review as I thought this was a trivial fix. The binary serialization also serializes the Origin field.</div></div></div></div></blockquote></div></div></div><div dir="auto"><div dir="auto">LG to be consistent with binary serialization, the same question applies to binary serialization.</div><div dir="auto"><br></div><div dir="auto">Again, the change itself seems fine, just nitpicking on whether putting origin into a symbol at that level makes sense. </div></div><div dir="auto"><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Am I missing something? </div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 21, 2018 at 3:06 PM Eric Liu via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" rel="noreferrer" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ioeric<br>
Date: Fri Sep 21 06:04:57 2018<br>
New Revision: 342730<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=342730&view=rev" rel="noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=342730&view=rev</a><br>
Log:<br>
[clangd] Remember to serialize symbol origin in YAML.<br>
<br>
Modified:<br>
clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp<br>
clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=342730&r1=342729&r2=342730&view=diff" rel="noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=342730&r1=342729&r2=342730&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Fri Sep 21 06:04:57 2018<br>
@@ -27,6 +27,7 @@ namespace yaml {<br>
<br>
using clang::clangd::Symbol;<br>
using clang::clangd::SymbolID;<br>
+using clang::clangd::SymbolOrigin;<br>
using clang::clangd::SymbolLocation;<br>
using clang::index::SymbolInfo;<br>
using clang::index::SymbolKind;<br>
@@ -65,6 +66,17 @@ struct NormalizedSymbolFlag {<br>
uint8_t Flag = 0;<br>
};<br>
<br>
+struct NormalizedSymbolOrigin {<br>
+ NormalizedSymbolOrigin(IO &) {}<br>
+ NormalizedSymbolOrigin(IO &, SymbolOrigin O) {<br>
+ Origin = static_cast<uint8_t>(O);<br>
+ }<br>
+<br>
+ SymbolOrigin denormalize(IO &) { return static_cast<SymbolOrigin>(Origin); }<br>
+<br>
+ uint8_t Origin = 0;<br>
+};<br>
+<br>
template <> struct MappingTraits<SymbolLocation::Position> {<br>
static void mapping(IO &IO, SymbolLocation::Position &Value) {<br>
IO.mapRequired("Line", Value.Line);<br>
@@ -102,6 +114,8 @@ template <> struct MappingTraits<Symbol><br>
MappingNormalization<NormalizedSymbolID, SymbolID> NSymbolID(IO, Sym.ID);<br>
MappingNormalization<NormalizedSymbolFlag, Symbol::SymbolFlag> NSymbolFlag(<br>
IO, Sym.Flags);<br>
+ MappingNormalization<NormalizedSymbolOrigin, SymbolOrigin> NSymbolOrigin(<br>
+ IO, Sym.Origin);<br>
IO.mapRequired("ID", NSymbolID->HexString);<br>
IO.mapRequired("Name", Sym.Name);<br>
IO.mapRequired("Scope", Sym.Scope);<br>
@@ -110,6 +124,7 @@ template <> struct MappingTraits<Symbol><br>
SymbolLocation());<br>
IO.mapOptional("Definition", Sym.Definition, SymbolLocation());<br>
IO.mapOptional("References", Sym.References, 0u);<br>
+ IO.mapOptional("Origin", NSymbolOrigin->Origin);<br>
IO.mapOptional("Flags", NSymbolFlag->Flag);<br>
IO.mapOptional("Signature", Sym.Signature);<br>
IO.mapOptional("CompletionSnippetSuffix", Sym.CompletionSnippetSuffix);<br>
<br>
Modified: clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp?rev=342730&r1=342729&r2=342730&view=diff" rel="noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp?rev=342730&r1=342729&r2=342730&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp (original)<br>
+++ clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp Fri Sep 21 06:04:57 2018<br>
@@ -7,6 +7,7 @@<br>
//<br>
//===----------------------------------------------------------------------===//<br>
<br>
+#include "index/Index.h"<br>
#include "index/Serialization.h"<br>
#include "index/SymbolYAML.h"<br>
#include "llvm/Support/ScopedPrinter.h"<br>
@@ -35,6 +36,7 @@ CanonicalDeclaration:<br>
End:<br>
Line: 1<br>
Column: 1<br>
+Origin: 4<br>
Flags: 1<br>
Documentation: 'Foo doc'<br>
ReturnType: 'int'<br>
@@ -82,6 +84,7 @@ TEST(SerializationTest, YAMLConversions)<br>
EXPECT_EQ(Sym1.Documentation, "Foo doc");<br>
EXPECT_EQ(Sym1.ReturnType, "int");<br>
EXPECT_EQ(Sym1.CanonicalDeclaration.FileURI, "file:///path/foo.h");<br>
+ EXPECT_EQ(Sym1.Origin, SymbolOrigin::Static);<br>
EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion);<br>
EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated);<br>
EXPECT_THAT(Sym1.IncludeHeaders,<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" rel="noreferrer" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="m_-1656042347549317210m_7291527606658289420m_-3757521949622962162m_-7757223667409494260gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</blockquote></div></div></div>
</blockquote></div></div></div></blockquote></div></div></div>