[clang-tools-extra] r342730 - [clangd] Remember to serialize symbol origin in YAML.
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 25 09:58:18 PDT 2018
Eric Liu <ioeric at google.com> schrieb am Di., 25. Sep. 2018, 01:22:
> On Tue, Sep 25, 2018 at 6:34 AM Ilya Biryukov <ibiryukov at google.com>
> wrote:
>
>> Why would we want to serialize the origin?
>>
> We only serialize and deserialize for the static index, it does not seem
>> to be useful to serialize origin in that scenario.
>>
> We serialize Origin because it's a property of Symbol? The origin for the
> current YAML symbols is defaulted to "unknown".
>
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.
> 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.
>
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.
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.
If we will have a use case for serializing the latter entity(with origin)
for any reason, we might add that separately.
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.
> I checked this in without review as I thought this was a trivial fix. The
> binary serialization also serializes the Origin field.
>
LG to be consistent with binary serialization, the same question applies to
binary serialization.
Again, the change itself seems fine, just nitpicking on whether putting
origin into a symbol at that level makes sense.
>> Am I missing something?
>>
>
>> On Fri, Sep 21, 2018 at 3:06 PM Eric Liu via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> Author: ioeric
>>> Date: Fri Sep 21 06:04:57 2018
>>> New Revision: 342730
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=342730&view=rev
>>> Log:
>>> [clangd] Remember to serialize symbol origin in YAML.
>>>
>>> Modified:
>>> clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
>>> clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
>>>
>>> Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=342730&r1=342729&r2=342730&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original)
>>> +++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Fri Sep 21
>>> 06:04:57 2018
>>> @@ -27,6 +27,7 @@ namespace yaml {
>>>
>>> using clang::clangd::Symbol;
>>> using clang::clangd::SymbolID;
>>> +using clang::clangd::SymbolOrigin;
>>> using clang::clangd::SymbolLocation;
>>> using clang::index::SymbolInfo;
>>> using clang::index::SymbolKind;
>>> @@ -65,6 +66,17 @@ struct NormalizedSymbolFlag {
>>> uint8_t Flag = 0;
>>> };
>>>
>>> +struct NormalizedSymbolOrigin {
>>> + NormalizedSymbolOrigin(IO &) {}
>>> + NormalizedSymbolOrigin(IO &, SymbolOrigin O) {
>>> + Origin = static_cast<uint8_t>(O);
>>> + }
>>> +
>>> + SymbolOrigin denormalize(IO &) { return
>>> static_cast<SymbolOrigin>(Origin); }
>>> +
>>> + uint8_t Origin = 0;
>>> +};
>>> +
>>> template <> struct MappingTraits<SymbolLocation::Position> {
>>> static void mapping(IO &IO, SymbolLocation::Position &Value) {
>>> IO.mapRequired("Line", Value.Line);
>>> @@ -102,6 +114,8 @@ template <> struct MappingTraits<Symbol>
>>> MappingNormalization<NormalizedSymbolID, SymbolID> NSymbolID(IO,
>>> Sym.ID);
>>> MappingNormalization<NormalizedSymbolFlag, Symbol::SymbolFlag>
>>> NSymbolFlag(
>>> IO, Sym.Flags);
>>> + MappingNormalization<NormalizedSymbolOrigin, SymbolOrigin>
>>> NSymbolOrigin(
>>> + IO, Sym.Origin);
>>> IO.mapRequired("ID", NSymbolID->HexString);
>>> IO.mapRequired("Name", Sym.Name);
>>> IO.mapRequired("Scope", Sym.Scope);
>>> @@ -110,6 +124,7 @@ template <> struct MappingTraits<Symbol>
>>> SymbolLocation());
>>> IO.mapOptional("Definition", Sym.Definition, SymbolLocation());
>>> IO.mapOptional("References", Sym.References, 0u);
>>> + IO.mapOptional("Origin", NSymbolOrigin->Origin);
>>> IO.mapOptional("Flags", NSymbolFlag->Flag);
>>> IO.mapOptional("Signature", Sym.Signature);
>>> IO.mapOptional("CompletionSnippetSuffix",
>>> Sym.CompletionSnippetSuffix);
>>>
>>> Modified: clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp?rev=342730&r1=342729&r2=342730&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
>>> (original)
>>> +++ clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp Fri
>>> Sep 21 06:04:57 2018
>>> @@ -7,6 +7,7 @@
>>> //
>>>
>>> //===----------------------------------------------------------------------===//
>>>
>>> +#include "index/Index.h"
>>> #include "index/Serialization.h"
>>> #include "index/SymbolYAML.h"
>>> #include "llvm/Support/ScopedPrinter.h"
>>> @@ -35,6 +36,7 @@ CanonicalDeclaration:
>>> End:
>>> Line: 1
>>> Column: 1
>>> +Origin: 4
>>> Flags: 1
>>> Documentation: 'Foo doc'
>>> ReturnType: 'int'
>>> @@ -82,6 +84,7 @@ TEST(SerializationTest, YAMLConversions)
>>> EXPECT_EQ(Sym1.Documentation, "Foo doc");
>>> EXPECT_EQ(Sym1.ReturnType, "int");
>>> EXPECT_EQ(Sym1.CanonicalDeclaration.FileURI, "file:///path/foo.h");
>>> + EXPECT_EQ(Sym1.Origin, SymbolOrigin::Static);
>>> EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion);
>>> EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated);
>>> EXPECT_THAT(Sym1.IncludeHeaders,
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>> --
>> Regards,
>> Ilya Biryukov
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180925/7c77eff4/attachment.html>
More information about the cfe-commits
mailing list