From f5d80e90a39a1c677f70a6ee62440906049681c7 Mon Sep 17 00:00:00 2001 From: Bartek Kryza Date: Sat, 7 May 2022 19:59:13 +0200 Subject: [PATCH] Fixed class template handling --- .../plantuml/class_diagram_generator.cc | 2 +- src/class_diagram/model/class.cc | 4 +- src/class_diagram/model/class_template.cc | 56 ++++++++++--- src/class_diagram/model/class_template.h | 31 ++++++- src/class_diagram/model/diagram.cc | 39 +++++---- .../visitor/translation_unit_visitor.cc | 83 ++++++++++++------- src/common/model/nested_trait.h | 4 +- src/common/model/path.h | 2 +- tests/t00014/t00014.cc | 31 +++---- tests/t00014/test_case.h | 12 +-- tests/t00027/test_case.h | 12 +-- 11 files changed, 185 insertions(+), 91 deletions(-) diff --git a/src/class_diagram/generators/plantuml/class_diagram_generator.cc b/src/class_diagram/generators/plantuml/class_diagram_generator.cc index 290eb540..ff04e5c0 100644 --- a/src/class_diagram/generators/plantuml/class_diagram_generator.cc +++ b/src/class_diagram/generators/plantuml/class_diagram_generator.cc @@ -64,7 +64,7 @@ void generator::generate_alias(const class_ &c, std::ostream &ostr) const else full_name = c.full_name(); - //util::replace_all(full_name, "std::basic_string", "std::string"); + // util::replace_all(full_name, "std::basic_string", "std::string"); if (full_name.empty()) full_name = "<>"; diff --git a/src/class_diagram/model/class.cc b/src/class_diagram/model/class.cc index f295118c..dc2baeae 100644 --- a/src/class_diagram/model/class.cc +++ b/src/class_diagram/model/class.cc @@ -87,7 +87,8 @@ std::string class_::base_template() const { return base_template_full_name_; } bool operator==(const class_ &l, const class_ &r) { - return l.full_name() == r.full_name(); + return (l.name_and_ns() == r.name_and_ns()) && + (l.templates_ == r.templates_); } void class_::add_type_alias(type_alias &&ta) @@ -148,6 +149,7 @@ std::ostringstream &class_::render_template_params( ostr << fmt::format("<{}>", fmt::join(tnames, ",")); } + return ostr; } diff --git a/src/class_diagram/model/class_template.cc b/src/class_diagram/model/class_template.cc index bb0d1a1f..a1494852 100644 --- a/src/class_diagram/model/class_template.cc +++ b/src/class_diagram/model/class_template.cc @@ -24,27 +24,33 @@ namespace clanguml::class_diagram::model { class_template::class_template(const std::string &type, const std::string &name, const std::string &default_value, bool is_variadic) - : name_{name} - , default_value_{default_value} + : default_value_{default_value} , is_variadic_{is_variadic} { + set_name(name); set_type(type); - if (is_variadic) - name_ = name_ + "..."; } -void class_template::set_type(const std::string &type) { - type_ = type; - // TODO: Add a configurable mapping for simplifying non-interesting - // std templates - util::replace_all(type_, "std::basic_string", "std::string"); -} +void class_template::set_type(const std::string &type) { type_ = type; } std::string class_template::type() const { return type_; } -void class_template::set_name(const std::string &name) { name_ = name; } +void class_template::set_name(const std::string &name) +{ + name_ = name; + // TODO: Add a configurable mapping for simplifying non-interesting + // std templates + util::replace_all(name_, "std::basic_string", "std::string"); + util::replace_all(name_, "std::basic_string", "std::wstring"); +} -std::string class_template::name() const { return name_; } +std::string class_template::name() const +{ + if (is_variadic_) + return name_ + "..."; + + return name_; +} void class_template::set_default_value(const std::string &value) { @@ -60,9 +66,33 @@ void class_template::is_variadic(bool is_variadic) noexcept bool class_template::is_variadic() const noexcept { return is_variadic_; } +bool class_template::is_specialization_of(const class_template &ct) const +{ + if ((ct.is_template_parameter() || ct.is_template_template_parameter()) && + !is_template_parameter()) + return true; + + return false; +} + bool operator==(const class_template &l, const class_template &r) { - return (l.name() == r.name()) && (l.type() == r.type()); + bool res{false}; + + if (l.is_template_parameter() != r.is_template_parameter()) + return res; + + if (l.is_template_parameter()) { + // If this is a template parameter (e.g. 'typename T' or 'typename U' + // we don't actually care what it is called + res = (l.is_variadic() == r.is_variadic()) && + (l.default_value() == r.default_value()); + } + else + res = (l.name() == r.name()) && (l.type() == r.type()) && + (l.default_value() == r.default_value()); + + return res && (l.template_params_ == r.template_params_); } bool operator!=(const class_template &l, const class_template &r) diff --git a/src/class_diagram/model/class_template.h b/src/class_diagram/model/class_template.h index 36cf42d8..0a014c80 100644 --- a/src/class_diagram/model/class_template.h +++ b/src/class_diagram/model/class_template.h @@ -41,11 +41,11 @@ public: void is_variadic(bool is_variadic) noexcept; bool is_variadic() const noexcept; + bool is_specialization_of(const class_template &ct) const; + friend bool operator==(const class_template &l, const class_template &r); friend bool operator!=(const class_template &l, const class_template &r); - std::vector template_params_; - bool is_template_parameter() const { return is_template_parameter_; } void is_template_parameter(bool is_template_parameter) @@ -53,14 +53,41 @@ public: is_template_parameter_ = is_template_parameter; } + bool is_template_template_parameter() const + { + return is_template_template_parameter_; + } + + void is_template_template_parameter(bool is_template_template_parameter) + { + is_template_template_parameter_ = is_template_template_parameter; + } + std::string to_string( const clanguml::common::model::namespace_ &using_namespace) const; + std::vector template_params_; + private: + /// Represents the type of non-type template parameters + /// e.g. 'int' std::string type_; + + /// The name of the parameter (e.g. 'T' or 'N') std::string name_; + + /// Default value of the template parameter std::string default_value_; + + /// Whether the template parameter is a regular template parameter + /// When false, it is a non-type template parameter bool is_template_parameter_{false}; + + /// Whether the template parameter is a template template parameter + /// Can only be true when is_template_parameter_ is true + bool is_template_template_parameter_{false}; + + /// Whether the template parameter is variadic bool is_variadic_{false}; }; } diff --git a/src/class_diagram/model/diagram.cc b/src/class_diagram/model/diagram.cc index 527f6bac..e964244c 100644 --- a/src/class_diagram/model/diagram.cc +++ b/src/class_diagram/model/diagram.cc @@ -58,7 +58,7 @@ diagram::get(const std::string &full_name) const bool diagram::has_class(const class_ &c) const { return std::any_of(classes_.cbegin(), classes_.cend(), - [&c](const auto &cc) { return cc.get().full_name() == c.full_name(); }); + [&c](const auto &cc) { return cc.get() == c; }); } bool diagram::has_enum(const enum_ &e) const @@ -109,30 +109,37 @@ void diagram::add_package(std::unique_ptr &&p) void diagram::add_class(std::unique_ptr &&c) { + const auto base_name = c->name(); + const auto full_name = c->full_name(false); + LOG_DBG("Adding class: {}::{}, {}", c->get_namespace().to_string(), - c->name(), c->full_name()); + base_name, full_name); - if (util::contains(c->name(), "::")) - throw std::runtime_error("Name cannot contain namespace: " + c->name()); + if (util::contains(base_name, "::")) + throw std::runtime_error("Name cannot contain namespace: " + base_name); - if (util::contains(c->name(), "<")) - throw std::runtime_error("Name cannot contain <: " + c->name()); + if (util::contains(base_name, "<")) + throw std::runtime_error("Name cannot contain <: " + base_name); - if (util::contains(c->name(), "*")) - throw std::runtime_error("Name cannot contain *: " + c->name()); + if (util::contains(base_name, "*")) + throw std::runtime_error("Name cannot contain *: " + base_name); + + const auto ns = c->get_relative_namespace(); + auto name = base_name; + auto name_with_ns = c->name_and_ns(); + auto name_and_ns = ns | name; if (!has_class(*c)) { - classes_.emplace_back(*c); - auto ns = c->get_relative_namespace(); - auto name = c->name(); + classes_.push_back(type_safe::ref(*c)); + add_element(ns, std::move(c)); - ns |= name; - const auto ccc = get_element(ns); - assert(ccc.value().name() == name); + + const auto &el = get_element(name_and_ns).value(); + assert(el.name() == name); + assert(el.get_relative_namespace() == ns); } else - LOG_DBG( - "Class {} ({}) already in the model", c->name(), c->full_name()); + LOG_DBG("Class {} ({}) already in the model", base_name, full_name); } void diagram::add_enum(std::unique_ptr &&e) diff --git a/src/class_diagram/visitor/translation_unit_visitor.cc b/src/class_diagram/visitor/translation_unit_visitor.cc index 6f14a5a4..e3c00faf 100644 --- a/src/class_diagram/visitor/translation_unit_visitor.cc +++ b/src/class_diagram/visitor/translation_unit_visitor.cc @@ -216,6 +216,8 @@ void translation_unit_visitor::process_type_alias_template( resolve_alias(at.type_alias().underlying_type())), nested_relationships); + assert(tinst); + tinst->is_alias(true); if (tinst->get_namespace().is_empty()) @@ -774,6 +776,8 @@ bool translation_unit_visitor::process_field_with_template_instantiation( LOG_DBG("Created template instantiation: {}", tinst.full_name()); + assert(tinst_ptr); + ctx.diagram().add_class(std::move(tinst_ptr)); } else if (!nested_relationships.empty()) { @@ -1249,7 +1253,7 @@ void translation_unit_visitor:: // First check if tinst already exists auto tinst_ptr = build_template_instantiation( template_instantiation_type, nested_relationships); - auto &tinst = *tinst_ptr; + const auto &tinst = *tinst_ptr; relationship rr{relationship_t::kDependency, tinst.full_name()}; LOG_DBG("Adding field dependency relationship {} {} {} " @@ -1259,7 +1263,7 @@ void translation_unit_visitor:: c.add_relationship(std::move(rr)); - if (ctx.diagram().should_include(c)) + if (ctx.diagram().should_include(tinst)) ctx.diagram().add_class(std::move(tinst_ptr)); } } @@ -1268,20 +1272,40 @@ void translation_unit_visitor:: void translation_unit_visitor::process_template_type_parameter( const cppast::cpp_template_type_parameter &t, class_ &parent) { - parent.add_template({"", t.name(), "", t.is_variadic()}); + class_template ct; + ct.set_type(""); + ct.is_template_parameter(true); + ct.set_name(t.name()); + ct.set_default_value(""); + ct.is_variadic(t.is_variadic()); + + parent.add_template(std::move(ct)); } void translation_unit_visitor::process_template_nontype_parameter( const cppast::cpp_non_type_template_parameter &t, class_ &parent) { - parent.add_template( - {cppast::to_string(t.type()), t.name(), "", t.is_variadic()}); + class_template ct; + ct.set_type(cppast::to_string(t.type())); + ct.is_template_parameter(false); + ct.set_name(t.name()); + ct.set_default_value(""); + ct.is_variadic(t.is_variadic()); + + parent.add_template(std::move(ct)); } void translation_unit_visitor::process_template_template_parameter( const cppast::cpp_template_template_parameter &t, class_ &parent) { - parent.add_template({"", t.name() + "<>"}); + class_template ct; + ct.set_type(""); + ct.is_template_template_parameter(true); + ct.set_name(t.name() + "<>"); + ct.set_default_value(""); + ct.is_variadic(t.is_variadic()); + + parent.add_template(std::move(ct)); } void translation_unit_visitor::process_friend(const cppast::cpp_friend &f, @@ -1637,11 +1661,6 @@ std::unique_ptr translation_unit_visitor::build_template_instantiation( tinst.is_template_instantiation(true); tinst.is_alias(t_is_alias); - if (tinst.full_name().substr(0, tinst.full_name().find('<')).find("::") == - std::string::npos) { - tinst.set_name(name); - } - if (t.arguments_exposed()) { auto arg_index = 0U; // We can figure this only when we encounter variadic param in @@ -1671,7 +1690,7 @@ std::unique_ptr translation_unit_visitor::build_template_instantiation( ct); } - LOG_DBG("Adding template argument '{}'", ct.type()); + LOG_DBG("Adding template argument '{}'", ct.name()); tinst.add_template(std::move(ct)); } @@ -1695,23 +1714,21 @@ std::unique_ptr translation_unit_visitor::build_template_instantiation( int best_match = 0; // First try to find the best match for this template in partially // specialized templates - for (const auto &c : ctx.diagram().classes()) { + for (const auto c : ctx.diagram().classes()) { if (c->name_and_ns() == full_template_name && // TODO: handle variadic templates c->templates().size() == tinst.templates().size()) { int tmp_match = 0; - for (int i = 0; i < c->templates().size(); i++) { + for (int i = 0; i < tinst.templates().size(); i++) { const auto &tinst_i = tinst.templates().at(i); const auto &c_i = c->templates().at(i); - if ((c_i == tinst_i) && !c_i.is_template_parameter()) { + if (c_i == tinst_i) { tmp_match++; } - else if (c_i.is_template_parameter() || - tinst_i.is_template_parameter()) { - // continue + else if (tinst_i.is_specialization_of(c_i)) { + continue; } else { - // Non-free types are not compatible tmp_match = 0; break; } @@ -1754,11 +1771,11 @@ bool translation_unit_visitor::build_template_instantiation_add_base_classes( } if (add_template_argument_as_base_class) { - LOG_DBG("Adding template argument as base class '{}'", ct.type()); + LOG_DBG("Adding template argument as base class '{}'", ct.name()); class_parent cp; cp.set_access(access_t::kPublic); - cp.set_name(ct.type()); + cp.set_name(ct.name()); tinst.add_parent(std::move(cp)); } @@ -1779,7 +1796,7 @@ void translation_unit_visitor:: .expression() .as_string()); - LOG_DBG("Template argument is an expression {}", ct.type()); + LOG_DBG("Template argument is an expression {}", ct.name()); } void translation_unit_visitor:: @@ -1789,20 +1806,29 @@ void translation_unit_visitor:: class_template &ct, found_relationships_t &nested_relationships, common::model::relationship_t relationship_hint) { - ct.set_type(cppast::to_string(targ.type().value())); + ct.set_name(cppast::to_string(targ.type().value())); - LOG_DBG("Template argument is a type {}", ct.type()); + LOG_DBG("Template argument is a type {}", ct.name()); auto fn = cx::util::full_name( cppast::remove_cv(cx::util::unreferenced(targ.type().value())), ctx.entity_index(), false); auto [fn_ns, fn_name] = cx::util::split_ns(fn); + auto template_argument_kind = targ.type().value().kind(); - if (targ.type().value().kind() == + if (template_argument_kind == cppast::cpp_type_kind::unexposed_t) { + // Here we're on our own - just make a best guess + if (!fn.empty() && !util::contains(fn, "<") && + !util::contains(fn, ":") && std::isupper(fn.at(0))) + ct.is_template_parameter(true); + else + ct.is_template_parameter(false); + } + else if (template_argument_kind == cppast::cpp_type_kind::template_parameter_t) { ct.is_template_parameter(true); } - else if (targ.type().value().kind() == + else if (template_argument_kind == cppast::cpp_type_kind::template_instantiation_t) { const auto &nested_template_parameter = @@ -1822,6 +1848,8 @@ void translation_unit_visitor:: ? std::make_optional(&tinst) : parent); + assert(nested_tinst); + relationship tinst_dependency{ relationship_t::kDependency, nested_tinst->full_name()}; @@ -1861,8 +1889,7 @@ void translation_unit_visitor:: fn, tinst.full_name(), tinst_dependency.destination()); } } - else if (targ.type().value().kind() == - cppast::cpp_type_kind::user_defined_t) { + else if (template_argument_kind == cppast::cpp_type_kind::user_defined_t) { relationship tinst_dependency{relationship_t::kDependency, cx::util::full_name( cppast::remove_cv(cx::util::unreferenced(targ.type().value())), diff --git a/src/common/model/nested_trait.h b/src/common/model/nested_trait.h index 8894d4e1..b4e79cfe 100644 --- a/src/common/model/nested_trait.h +++ b/src/common/model/nested_trait.h @@ -56,8 +56,8 @@ public: { assert(p); - LOG_DBG( - "Adding nested element {} at path {}", p->name(), path.to_string()); + LOG_DBG("Adding nested element {} at path '{}'", p->name(), + path.to_string()); if (path.is_empty()) { add_element(std::move(p)); diff --git a/src/common/model/path.h b/src/common/model/path.h index 555b5085..44571bb3 100644 --- a/src/common/model/path.h +++ b/src/common/model/path.h @@ -39,7 +39,7 @@ public: std::copy(begin, end, std::back_inserter(path_)); } - path(const path &right) noexcept = default; + path(const path &right) { path_ = right.path_; } path &operator=(const path &right) noexcept = default; diff --git a/tests/t00014/t00014.cc b/tests/t00014/t00014.cc index 8b13a7b2..18929c44 100644 --- a/tests/t00014/t00014.cc +++ b/tests/t00014/t00014.cc @@ -27,22 +27,25 @@ template struct A { P p; }; -template using AString = A; -template using AStringPtr = A>; - -template using VectorPtr = std::unique_ptr>; -template using APtr = std::unique_ptr>; -template using ASharedPtr = std::shared_ptr>; - template using AAPtr = - std::unique_ptr, A>>; - -template using GeneralCallback = std::function; -using VoidCallback = GeneralCallback<>; - struct B { std::string value; }; +template using AString = A; +template using AStringPtr = A>; + +template +using PairPairBA = std::pair>, long>; + +template using VectorPtr = std::unique_ptr>; +template using APtr = std::unique_ptr>; +template using ASharedPtr = std::shared_ptr>; +template +using AAPtr = std::unique_ptr, A>>; + +template using GeneralCallback = std::function; +using VoidCallback = GeneralCallback<>; + using BVector = std::vector; using BVector2 = BVector; @@ -50,13 +53,11 @@ using AIntString = AString; using AStringString = AString; using BStringString = AStringString; -template using PairPairBA = std::pair>, long>; - class R { PairPairBA bapair; APtr abool; - AAPtr aboolfloat; + AAPtr aboolfloat; ASharedPtr afloat; A boolstring; AStringPtr floatstring; diff --git a/tests/t00014/test_case.h b/tests/t00014/test_case.h index 2530148d..e7dc0533 100644 --- a/tests/t00014/test_case.h +++ b/tests/t00014/test_case.h @@ -38,7 +38,7 @@ TEST_CASE("t00014", "[test-case][class]") REQUIRE_THAT(puml, IsClassTemplate("A", "T,std::string")); REQUIRE_THAT(puml, IsClassTemplate("A", "T,std::unique_ptr")); REQUIRE_THAT(puml, IsClassTemplate("A", "double,T")); - REQUIRE_THAT(puml, IsClassTemplate("A", "long,U")); + REQUIRE_THAT(puml, !IsClassTemplate("A", "long,U")); REQUIRE_THAT(puml, IsClassTemplate("A", "long,T")); REQUIRE_THAT(puml, IsClassTemplate("A", "long,bool")); REQUIRE_THAT(puml, IsClassTemplate("A", "double,bool")); @@ -53,10 +53,9 @@ TEST_CASE("t00014", "[test-case][class]") REQUIRE_THAT(puml, IsInstantiation(_A("A"), _A("A"))); REQUIRE_THAT(puml, IsInstantiation(_A("A"), _A("A"))); REQUIRE_THAT(puml, IsInstantiation(_A("A"), _A("A"))); - // TODO: Fix matching partial template specializations with differently - // named template paremeters - // REQUIRE_THAT( - // puml, IsInstantiation(_A("A"), _A("A"))); + + REQUIRE_THAT(puml, IsInstantiation(_A("A"), _A("A"))); + REQUIRE_THAT(puml, !IsInstantiation(_A("A"), _A("A"))); REQUIRE_THAT( puml, IsInstantiation(_A("A"), _A("A"))); REQUIRE_THAT( @@ -79,7 +78,8 @@ TEST_CASE("t00014", "[test-case][class]") REQUIRE_THAT(puml, IsAggregation(_A("R"), _A("A"), "-bapair")); REQUIRE_THAT( puml, IsAggregation(_A("R"), _A("A"), "-aboolfloat")); - REQUIRE_THAT(puml, IsAssociation(_A("R"), _A("A"), "-afloat")); + REQUIRE_THAT( + puml, IsAssociation(_A("R"), _A("A"), "-afloat")); REQUIRE_THAT( puml, IsAggregation(_A("R"), _A("A"), "-boolstring")); REQUIRE_THAT(puml, diff --git a/tests/t00027/test_case.h b/tests/t00027/test_case.h index f073b50b..0fd2e9ff 100644 --- a/tests/t00027/test_case.h +++ b/tests/t00027/test_case.h @@ -36,14 +36,14 @@ TEST_CASE("t00027", "[test-case][class]") REQUIRE_THAT(puml, EndsWith("@enduml\n")); REQUIRE_THAT(puml, IsAbstractClass(_A("Shape"))); REQUIRE_THAT(puml, IsAbstractClass(_A("ShapeDecorator"))); - REQUIRE_THAT(puml, IsClassTemplate("Line", "T<>")); - REQUIRE_THAT(puml, IsClassTemplate("Text", "T<>")); - REQUIRE_THAT(puml, IsInstantiation(_A("Line>"), _A("Line"))); + REQUIRE_THAT(puml, IsClassTemplate("Line", "T<>...")); + REQUIRE_THAT(puml, IsClassTemplate("Text", "T<>...")); + REQUIRE_THAT(puml, IsInstantiation(_A("Line...>"), _A("Line"))); REQUIRE_THAT( - puml, IsInstantiation(_A("Line>"), _A("Line"))); - REQUIRE_THAT(puml, IsInstantiation(_A("Text>"), _A("Text"))); + puml, IsInstantiation(_A("Line...>"), _A("Line"))); + REQUIRE_THAT(puml, IsInstantiation(_A("Text...>"), _A("Text"))); REQUIRE_THAT( - puml, IsInstantiation(_A("Text>"), _A("Text"))); + puml, IsInstantiation(_A("Text...>"), _A("Text"))); REQUIRE_THAT( puml, IsAggregation(_A("Window"), _A("Line"), "+border"));