Page MenuHomePhabricator

[clangd] Construct SymbolSlab from YAML format.

Authored by hokein on Dec 13 2017, 5:41 AM.

Diff Detail


Event Timeline

hokein created this revision.Dec 13 2017, 5:41 AM

Nice! just nits

32 ↗(On Diff #126742)

what does NP stand for?

44 ↗(On Diff #126742)

Nit: for readability, I suggest you encode as a hex string instead.

We don't yet have operator<< for SymbolID, but I think we should, and it should be consistent.
So preferably define operator<< for SymbolID, and use it here, rather than just defining everything here.

1 ↗(On Diff #126742)

nit: SymbolFromYAML.h seems slightly off for the file, because you support both conversions (and also multiple symbols)

Consider just YAML.h?

8 ↗(On Diff #126742)

File comment, describing what's going on here, use cases.

In particular, I'd note that the format is designed for simplicity but isn't a suitable/efficient store and isn't intended for real use by end-users.

23 ↗(On Diff #126742)

We may have multiple slabs, e.g. if we want to dump the dynamic index.
If we dump each slab and concatenate the resulting strings, is the result valid YAML (containing all symbols)?
If so, then this interface is good, but it may be worth a comment // The YAML is safe to concatenate if you have multiple slabs.

108 ↗(On Diff #126742)

round-trip is good to test the serialization, but can you add a separate test case loading a simple symbol from an actual YAML string?

hokein updated this revision to Diff 126909.Dec 14 2017, 1:30 AM
hokein marked 5 inline comments as done.

Address the review comments.

Thanks for the comments!

32 ↗(On Diff #126742)

Ah, P is a typo here. N stands for Normalized. Renamed it to NormalizedXXX.

44 ↗(On Diff #126742)

Good idea. Done.

1 ↗(On Diff #126742)

YAML seems too general. Named it SymbolYAML indicating the conversion between Symbol and YAML.

sammccall accepted this revision.Dec 14 2017, 2:39 AM
sammccall added inline comments.
17 ↗(On Diff #126909)

This isn't needed anymore (if you remove the friend below)

57 ↗(On Diff #126909)

I think this no longer needs to be a friend.

This revision is now accepted and ready to land.Dec 14 2017, 2:39 AM
hokein updated this revision to Diff 126927.Dec 14 2017, 3:45 AM
hokein marked 2 inline comments as done.

MappingTraits is not Symbol's friend.

This revision was automatically updated to reflect the committed changes.