[clang-tools-extra] r342730 - [clangd] Remember to serialize symbol origin in YAML.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 26 05:25:36 PDT 2018
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.
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.
On Tue, Sep 25, 2018 at 6:58 PM Ilya Biryukov <ibiryukov at google.com> wrote:
>
>
> 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/20180926/e65e102b/attachment.html>
More information about the cfe-commits
mailing list