Cheers, Ondrej On 19.1.2017 13:26, Dodji Seketeli wrote:
Ondrej Oprala <ondrej.oprala@gmail.com> a écrit:Hi Dodji,Hello Ondrej, Thank you for a great patch! I like it. I raise some mostly cosmetic points in my comments below. I am sure these should be resolved fairly quickly. Please let me know. [...]* tools/abidiff.cc: Add the new option '--no-corpus-path'.Please add documentation for the new --annotate option to the doc/manuals/abidiff.rst file.* tools/abidw.cc: Likewise. Also add the '--annotate' option.Please add documentation for the new --annotate option to the doc/manuals/abidw.rst file. [...]diff --git a/src/abg-writer.cc b/src/abg-writer.cc[...]+ bool + get_annotate()Please an apidoc comment to this accessor.+ {return m_annotate;} +[...]+/// Annotate a declaration in form of an ABIXML comment. +/// This function is further specialized for declarations +/// and types with special requirements. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is validThe parameter decl_sptr doesn't have an apidoc comment string. Please add one that says that it has to be of type decl_base_sptr (or inherit from it) for this particular function template to be instantiated. I guess you might say that its comment string is implicit, given the brief first line description of the function. Yes, you would be right. But then in the generated online apidoc, having *all* parameters documented does have value. It makes the doc better for the reader because she has confirmation that the first parameter is for the declaration that the brief description talked about. She goes from 'hmmh, this decl_sptr parameter must be the declaration' to "right, this decl_sptr parameter *is* the declaration." Said otherwise, in this matters (documentation), redundancy (to some extent) is a good thing. Also, please use the @tparam doxygen tag to document the template parameter type. This is tedious, I agree, but then the result is that we have a (hopefully) high quality apidoc that only few Free Software libraries out there can claim to have. For the joy of (potential) users of the library. :-)+template <typename T> +static bool +annotate(const T& decl_sptr, + write_context& ctxt, + unsigned indent) +{[...]+/// Annotate a type_base declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template <> +bool +annotate(const type_base_sptr& type_sptr, + write_context& ctxt, + unsigned indent)Likewise, please add a comment for the type_sptr parameter. [...]+/// Annotate a qualified_type_def declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template <> +bool +annotate(const qualified_type_def_sptr& decl, + write_context& ctxt, + unsigned indent)Likewise for the decl parameter.+ +/// Annotate a type_decl declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template <> +bool +annotate(const type_decl_sptr& type_sptr, + write_context& ctxt, + unsigned indent)Likewise for the type_sptr parameter.+/// Annotate a array_type_def declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template <> +bool +annotate(const array_type_def_sptr& decl_sptr, + write_context& ctxt, + unsigned indent)Likewise for the decl_sptr parameter.+/// Annotate a function declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @param skip_first_parm if true, do not serialize the first +/// parameter of the function decl. +// +/// @return true iff decl is valid +bool +annotate(const function_decl_sptr& decl_sptr, + write_context& ctxt, + unsigned indent, + bool skip_first_parm)Likewise for the decl_sptr parameter. Also, I'd rather call this parameter fn_decl or something as specific as that. I think it self-documents the code more.+{ + if (!decl_sptr) + return false; + + if (!ctxt.get_annotate()) + return true; + + ostream& o = ctxt.get_ostream(); + + do_indent(o, indent); + o << "<!-- " + << xml::escape_xml_comment(get_type_declaration(decl_sptr->get_return_type())->get_name()) + << " " << xml::escape_xml_comment(decl_sptr->get_name()) << " ("; + + //todo skip_first_parm?What does this TODO comment mean? (I tend to not like TODO comments because I find that they clutter the code. And also, in general, they tend to stay there ad vitam eternam. So I'd rather either have a proper informative comment expressing the reservations we have or the particular assumptions we made here etc).+ vector<shared_ptr<function_decl::parameter> >::const_iterator pi = + decl_sptr->get_parameters().begin();Hmmh. For the case were decl_sptr is a method_decl, I think we should start iterating from the first non-implicit parameter (skipping the 'this' parameter, to be blunt). The type abigail::ir::function_decl has a get_first_non_implicit_parm member function that returns an iterator pointing to that parameter.+ for (skip_first_parm && pi != decl_sptr->get_parameters().end() ? ++pi: pi; + pi != decl_sptr->get_parameters().end(); + ++pi) + { + if (pi != decl_sptr->get_parameters().begin()) + o << ", "; + + o << xml::escape_xml_comment(get_type_declaration((*pi)->get_type())->get_name()); + } + o << ") -->\n"; + + return true; +} + +/// Annotate a function type in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @param skip_first_parm if true, do not serialize the first +/// parameter of the function decl. +// +/// @return true iff decl is valid +bool +annotate(const function_type_sptr& decl_sptr, + write_context& ctxt, + unsigned indent, + bool skip_first_parm)Likewise for the decl_sptr parameter. Please call it fn_type or something like that. Even more so that a function type is not a decl.+{ + if (!decl_sptr) + return false; + + if (!ctxt.get_annotate()) + return true; + + ostream& o = ctxt.get_ostream(); + + do_indent(o, indent); + o << "<!-- " + << xml::escape_xml_comment(get_type_declaration(decl_sptr->get_return_type())->get_name()) + << " ("; + + vector<shared_ptr<function_decl::parameter> >::const_iterator pi = + decl_sptr->get_parameters().begin();We should start iterating from the first non-implicit parameter here as well.+ for (skip_first_parm && pi != decl_sptr->get_parameters().end() ? ++pi: pi; + pi != decl_sptr->get_parameters().end(); + ++pi) + { + if (pi != decl_sptr->get_parameters().begin()) + o << ", "; + + o << xml::escape_xml_comment(get_type_declaration((*pi)->get_type())->get_name()); + } + o << ") -->\n"; + + return true; +}Please add a new line here.+/// Annotate a typedef declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template<> +bool +annotate(const typedef_decl_sptr& decl, + write_context& ctxt, + unsigned indent)Please add an apidoc comment for the decl parameter. [...]+/// Annotate a class declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template <> +bool +annotate(const class_decl_sptr& type_or_decl_sptr, + write_context& ctxt, + unsigned indent)Please add an apidoc comment for the type_or_decl_sptr parameter here. Please call it class_type or something as self-documenting as that. [...]+/// Annotate a union declaration in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template <> +bool +annotate(const union_decl_sptr& type_or_decl_sptr, + write_context& ctxt, + unsigned indent)Please add an apidoc comment for the type_or_decl_sptr parameter. Please call it union_type or something as self-documenting likethat. [...]+/// Annotate an elf symbol in form of an ABIXML comment, effectively writing +/// out its demangled form. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template<> +bool +annotate(const elf_symbol_sptr& sym, + write_context& ctxt, + unsigned indent)Please add an apidoc comment for the sym parameter.+{ + if (!sym) + return false; + + if (!ctxt.get_annotate()) + return true; + + ostream& o = ctxt.get_ostream(); + + do_indent(o, indent); + o << "<!-- " + << xml::escape_xml_comment(abigail::ir::demangle_cplus_mangled_name(sym->get_name())) + << " -->\n";It's super cool to demangle the symbol name here. Thank you for this idea.+ + return true; +} + +/// Annotate a namespace decl in form of an ABIXML comment. +/// +/// @param ctxt the context of the parsing. +/// +/// @param indent the amount of white space to indent to. +/// +/// @return true iff decl is valid +template<> +bool +annotate(const namespace_decl_sptr& decl, + write_context& ctxt, + unsigned indent) +{Please add an apidoc comment for the decl parameter here.+ if (!decl) + return false; + + if (!ctxt.get_annotate()) + return true; + + ostream& o = ctxt.get_ostream(); + + do_indent(o, indent); + o << "<!-- namespace " + << xml::escape_xml_comment(decl->get_name()) + << " -->\n"; + + return true; +} /// Write a location to the output stream. /// /// If the location is empty, nothing is written. @@ -1519,9 +1936,10 @@ write_translation_unit(const translation_unit& tu, bool write_translation_unit(const translation_unit& tu, unsigned indent, - std::ostream& out) + std::ostream& out, + const bool annotate)Please add an apidoc comment for the new annotate parameter. [...]@@ -1538,7 +1956,8 @@ write_translation_unit(const translation_unit& tu, bool write_translation_unit(const translation_unit& tu, unsigned indent, - const string& path) + const string& path, + const bool annotate)Likewise. [...]@@ -1758,9 +2183,18 @@ write_pointer_type_def(const pointer_type_def_sptr& decl,ostream& o = ctxt.get_ostream(); - do_indent(o, indent); type_base_sptr pointed_to_type = decl->get_pointed_to_type();+ + annotate(decl->get_canonical_type(), ctxt, indent); + /* + do_indent(o, indent); + o << "<!-- " + << get_pretty_representation(decl->get_canonical_type()) + << " -->\n"; + */Please remove this commented code altogether. [...]@@ -2485,6 +2952,7 @@ write_class_decl_opening_tag(const class_decl_sptr& decl, if (!decl) return false;+ //TODO annotateWhat do you mean by this comment? I mean, the write_class_decl function has the necessary code to annotate a class-decl. So why would you want to annotate here? [...]@@ -2559,6 +3027,7 @@ write_union_decl_opening_tag(const union_decl_sptr& decl, if (!decl) return false;+ //TODO annotateLikewise. [...]write_union_decl_opening_tag(decl, id, ctxt, indent, @@ -3050,6 +3524,7 @@ write_type_tparameter(const type_tparameter_sptr decl, write_context& ctxt, unsigned indent) { + //TODOPlease remove this comment.if (!decl) return false;@@ -3092,6 +3567,7 @@ write_non_type_tparameter(const shared_ptr<non_type_tparameter> decl, write_context& ctxt, unsigned indent) { + //TODOLikewise.if (!decl) return false;@@ -3128,6 +3604,7 @@ write_template_tparameter (const template_tparameter_sptr decl,write_context& ctxt, unsigned indent) { + //TODOLikewiseif (!decl) return false;@@ -3179,6 +3656,7 @@ write_type_composition(const shared_ptr<type_composition> decl, write_context& ctxt, unsigned indent) { + //TODO?Likewise.if (!decl) return false;@@ -3273,6 +3751,7 @@ static boolwrite_function_tdecl(const shared_ptr<function_tdecl> decl, write_context& ctxt, unsigned indent) { + //TODO??Likewise.if (!decl) return false;@@ -3319,6 +3798,7 @@ static boolwrite_class_tdecl(const shared_ptr<class_tdecl> decl, write_context& ctxt, unsigned indent) { + //TODO?Likewise. [...]@@ -3413,13 +3893,14 @@ create_archive_write_context(const string& archive_path) /// otherwise. static bool write_translation_unit_to_archive(const translation_unit& tu, - archive_write_ctxt& ctxt) + archive_write_ctxt& ctxt, + const bool annotate)Please add an apidoc string for the new annotate parameter. [...]@@ -3452,14 +3933,15 @@ write_translation_unit_to_archive(const translation_unit& tu, /// @return true upon successful completion, false otherwise. static bool write_corpus_to_archive(const corpus& corp, - archive_write_ctxt& ctxt) + archive_write_ctxt& ctxt, + const bool annotate)Likewise.@@ -3480,8 +3962,9 @@ write_corpus_to_archive(const corpus& corp, /// @return upon successful completion, false otherwise. static bool write_corpus_to_archive(const corpus& corp, - archive_write_ctxt_sptr ctxt) -{return write_corpus_to_archive(corp, *ctxt);} + archive_write_ctxt_sptr ctxt, + const bool annotate) +{return write_corpus_to_archive(corp, *ctxt, annotate);}Likewise./// Serialize the current corpus to disk in a file at a given path. /// @@ -3493,11 +3976,12 @@ write_corpus_to_archive(const corpus& corp, /// @return true upon successful completion, false otherwise. bool write_corpus_to_archive(const corpus& corp, - const string& path) + const string& path, + const bool annotate)Likewise. [...]@@ -3517,8 +4001,8 @@ write_corpus_to_archive(const corpus& corp) /// /// @return true upon successful completion, false otherwise. bool -write_corpus_to_archive(const corpus_sptr corp) -{return write_corpus_to_archive(*corp);} +write_corpus_to_archive(const corpus_sptr corp, bool annotate)Likewise. [...]@@ -3533,12 +4017,13 @@ write_corpus_to_archive(const corpus_sptr corp) bool write_corpus_to_native_xml(const corpus_sptr corpus, unsigned indent, - std::ostream& out) + std::ostream& out, + bool annotate)Likewise.@@ -3620,7 +4105,8 @@ write_corpus_to_native_xml(const corpus_sptr corpus, bool write_corpus_to_native_xml_file(const corpus_sptr corpus, unsigned indent, - const string& path) + const string& path, + bool annotate)Likewise. [...]@@ -3662,9 +4148,9 @@ using namespace abigail::ir; /// /// @param o the output stream to consider. void -dump(const decl_base_sptr d, std::ostream& o) +dump(const decl_base_sptr d, std::ostream& o, bool annotate)Likewise. [...]@@ -3673,8 +4159,8 @@ dump(const decl_base_sptr d, std::ostream& o) /// /// @param d the pointer to decl_base to serialize. void -dump(const decl_base_sptr d) -{dump(d, cerr);} +dump(const decl_base_sptr d, bool annotate)Likewise.@@ -3682,15 +4168,15 @@ dump(const decl_base_sptr d) /// /// @param o the output stream to serialize the @ref type_base to. void -dump(const type_base_sptr t, std::ostream& o) -{dump(get_type_declaration(t), o);} +dump(const type_base_sptr t, std::ostream& o, bool annotate) +{dump(get_type_declaration(t), o, annotate);}Likewise./// Serialize a pointer to type_base to stderr./// /// @param t the pointer to type_base to serialize. void -dump(const type_base_sptr t) -{dump(t, cerr);} +dump(const type_base_sptr t, bool annotate) +{dump(t, cerr, annotate);}Likewise./// Serialize a pointer to var_decl to an output stream. /// @@ -3698,9 +4184,9 @@ dump(const type_base_sptr t) /// /// @param o the output stream to serialize the @ref var_decl to. void -dump(const var_decl_sptr v, std::ostream& o) +dump(const var_decl_sptr v, std::ostream& o, bool annotate)Likewise. [...]@@ -3709,8 +4195,8 @@ dump(const var_decl_sptr v, std::ostream& o) /// /// @param v the pointer to var_decl to serialize. void -dump(const var_decl_sptr v) -{dump(v, cerr);} +dump(const var_decl_sptr v, bool annotate)Likewise. [...]@@ -3718,9 +4204,9 @@ dump(const var_decl_sptr v) /// /// @param o the outpout stream to serialize the translation_unit to. void -dump(const translation_unit& t, std::ostream& o) +dump(const translation_unit& t, std::ostream& o, bool annotate)Likewise. [...]@@ -3729,8 +4215,8 @@ dump(const translation_unit& t, std::ostream& o) /// /// @param t the translation_unit to serialize. void -dump(const translation_unit& t) -{dump(t, cerr);} +dump(const translation_unit& t, bool annotate)Likewise. [...]@@ -3738,20 +4224,20 @@ dump(const translation_unit& t) /// /// @param o the output stream to serialize the translation unit to. void -dump(const translation_unit_sptr t, std::ostream& o) +dump(const translation_unit_sptr t, std::ostream& o, const bool annotate)Likewise. [...]/// Serialize a pointer to @ref translation_unit to stderr. /// /// @param t the translation_unit_sptr to serialize. void -dump(const translation_unit_sptr t) +dump(const translation_unit_sptr t, const bool annotate)Likewise.index 33732fa..dc71dfb 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -23,6 +23,7 @@ endif[...]TESTS= \ runtestreaddwarf \ +runtestannotate \ runtestcanonicalizetypes.sh \ runtestdiffpkg \ runtestdifffilter \ @@ -74,6 +75,11 @@ runtestreaddwarf_SOURCES=test-read-dwarf.cc runtestreaddwarf_LDADD=libtestutils.la $(top_builddir)/src/libabigail.la runtestreaddwarf_LDFLAGS=-pthread+runtestannotate_SOURCES=test-annotate.cc+runtestannotate_LDADD=libtestutils.la $(top_builddir)/src/libabigail.la +#TODO ?What does this TODO comment mean? Please remove it.+runtestannotate_LDFLAGS=-pthreadAlso, why adding the -ptreahd flag here? Is that necessary? [...]+++ b/tests/test-annotate.cc[...]+int +main() +{ + using abigail::tests::get_src_dir; + using abigail::tests::get_build_dir; + using abigail::tools_utils::ensure_parent_dir_created; + + bool is_ok = true; + string in_elf_path, ref_report_path, out_report_path; + string abidw; + + abidw = string(get_build_dir()) + "/tools/abidw --annotate --no-show-locs --no-corpus-path";Just like what is done elsewhere in the source code, please avoid long lines like these that are longer than 80 characters. Please break this line up. [...]diff --git a/tools/abidiff.cc b/tools/abidiff.cc index 2ebe113..234abd6 100644 --- a/tools/abidiff.cc +++ b/tools/abidiff.cc @@ -81,6 +81,7 @@ struct options bool linux_kernel_mode; bool no_default_supprs; bool no_arch; + bool no_corpus; bool show_relative_offset_changes; bool show_stats_only; bool show_symtabs; @@ -113,6 +114,7 @@ struct options linux_kernel_mode(true), no_default_supprs(), no_arch(), + no_corpus(), show_relative_offset_changes(true), show_stats_only(), show_symtabs(), @@ -160,6 +162,7 @@ display_usage(const string& prog_name, ostream& out) << " --no-default-suppression don't load any " "default suppression specification\n" << " --no-architecture do not take architecture in account\n" + << " --no-corpus-path do not emit the path to the corpus in the output\n"There are two spaces between the name of the option and the beginning of the comment describing said option. Thus, please add one more space after the --no-corpus-path. Also, what this option does is that it doesn't take the path property of the corpus into account when analyzing the differences between the two copora. So please adjust the comment accordingly. [...]diff --git a/tools/abidw.cc b/tools/abidw.cc[...]@@ -125,6 +128,7 @@ display_usage(const string& prog_name, ostream& out) << " --noout do not emit anything after reading the binary\n" << " --suppressions|--suppr <path> specify a suppression file\n" << " --no-architecture do not emit architecture info in the output\n" + << " --no-corpus-path do not emit the path to the corpus in the output\n"Likewise, Thus, please add one more space after the --no-corpus-path.<< " --no-show-locs do now show location information\n" << " --check-alternate-debug-info <elf-path> check alternate debug info " "of <elf-path>\n" @@ -135,6 +139,7 @@ display_usage(const string& prog_name, ostream& out) << " --no-linux-kernel-mode don't consider the input binary as " "a Linux Kernel binary\n" << " --abidiff compare the loaded ABI against itself\n" + << " --annotate annotate type declarations\n"This rather annotates ABI artifacts (not just types) defined and used in the output to ease readability for humans. So please adjust the help string accordingly. Thank you again for this great patch. Cheers,
Attachment:
libabigail_20970_patch_review_1.tar.gz
Description: application/gzip