[PATCH] Adding Replacement serialization support

Edwin Vane edwin.vane at intel.com
Fri Aug 16 10:32:00 PDT 2013



================
Comment at: include/clang/Tooling/ReplacementsYaml.h:28
@@ +27,3 @@
+/// \brief The top-level YAML document that contains all Replacements.
+struct ReplacementsDocument {
+  /// A freeform chunk of text to describe the context of the replacements in
----------------
Manuel Klimek wrote:
> I'd vote against calling this "Document". To me, a document is something I edit :)
> Perhaps we can just call it yaml::Replacement? or YAMLReplacement?
I'm using YAML nomenclature here. A document is everything between --- and ... in a YAML file where a YAML file can have multiple documents. Given that reasoning do you still feel strongly about renaming it?

================
Comment at: include/clang/Tooling/ReplacementsYaml.h:31
@@ +30,3 @@
+  /// this document.
+  std::string Context;
+
----------------
Manuel Klimek wrote:
> I'd put some stronger idea of what we want here into the comment:
> // A chunk of text that gives context on how the replacement was generated (for example,
> // the translation unit parsed, the location of a template instantiation, etc).
> // This will be used for diagnostic purposes if the application of a replacement fails.
> 
> Furthermore, why don't we have one of those per replacement? If it's useful enough, I think we should really put it into the Replacement class as an optional field, and provide nice libs to populate it from a clang-tool.
> 
> Also, I'd perhaps call it DiagnosticInfo (but I don't really feel strongly here, just throwing out an idea).
If we put Context into Replacement I'd definitely want to name it something that implies it's an annotation and not part of Replacement comparison operators. Perhaps "metadata" a la LLVM?


http://llvm-reviews.chandlerc.com/D1422



More information about the cfe-commits mailing list