Improved handling of message call comments (#264)

src/common/visitor/translation_unit_visitor.h

@@ -265,23 +265,29 @@ public:
      * @param comment clang::RawComment pointer
      * @param de Reference to clang::DiagnosticsEngine
      * @param element Reference to element to be updated
+     * @return Comment with uml directives stripped from it
      */
-    void process_comment(const clang::RawComment *comment,
+    [[maybe_unused]] std::string process_comment(
-        clang::DiagnosticsEngine &de,
+        const clang::RawComment *comment, clang::DiagnosticsEngine &de,
         clanguml::common::model::decorated_element &e)
     {
-        if (comment != nullptr) {
+        if (comment == nullptr)
-            auto [it, inserted] = processed_comments_.emplace(comment);
+            return {};
 
-            if (!inserted)
+        auto [it, inserted] = processed_comments_.emplace(comment);
-                return;
 
-            // Process clang-uml decorators in the comments
+        if (!inserted)
-            // TODO: Refactor to use standard block comments processable by
+            return {};
-            //       clang comments
+
-            e.add_decorators(decorators::parse(
+        // Process clang-uml decorators in the comments
-                comment->getFormattedText(source_manager_, de)));
+        // TODO: Refactor to use standard block comments processable by
-        }
+        //       clang comments
+        const auto &[decorators, stripped_comment] =
+            decorators::parse(comment->getFormattedText(source_manager_, de));
+
+        e.add_decorators(decorators);
+
+        return stripped_comment;
     }
 
     /**
@@ -332,6 +338,12 @@ public:
      */
     const ConfigT &config() const { return config_; }
 
+protected:
+    std::set<const clang::RawComment *> &processed_comments()
+    {
+        return processed_comments_;
+    }
+
 private:
     // Reference to the output diagram model
     DiagramT &diagram_;

src/decorators/decorators.cc

@@ -187,10 +187,12 @@ std::shared_ptr<decorator> call::from_string(std::string_view c)
     return res;
 }
 
-std::vector<std::shared_ptr<decorator>> parse(
+std::pair<std::vector<std::shared_ptr<decorator>>, std::string> parse(
     std::string documentation_block, const std::string &clanguml_tag)
 {
     std::vector<std::shared_ptr<decorator>> res;
+    std::string stripped_comment;
+
     const std::string begin_tag{"@" + clanguml_tag};
     const auto begin_tag_size = begin_tag.size();
 
@@ -202,12 +204,20 @@ std::vector<std::shared_ptr<decorator>> parse(
     const std::string_view block_view{documentation_block};
 
     auto pos = block_view.find("@" + clanguml_tag + "{");
+
+    if (pos == std::string::npos) {
+        // This comment had no uml directives
+        return {{}, util::trim(documentation_block)};
+    }
+
+    size_t last_end_pos{0};
     while (pos < documentation_block.size()) {
         auto c_begin = pos + begin_tag_size;
-        auto c_end = documentation_block.find('}', c_begin);
+        auto c_end = block_view.find('}', c_begin);
 
-        if (c_end == std::string::npos)
+        if (c_end == std::string::npos) {
-            return res;
+            return {res, util::trim(stripped_comment)};
+        }
 
         auto com =
             decorator::from_string(block_view.substr(c_begin + 1, c_end - 2));
@@ -215,10 +225,15 @@ std::vector<std::shared_ptr<decorator>> parse(
         if (com)
             res.emplace_back(std::move(com));
 
+        const auto in_between_length = pos - last_end_pos;
+        stripped_comment += block_view.substr(last_end_pos, in_between_length);
+
+        last_end_pos = pos + (c_end - c_begin + begin_tag_size + 1);
+
         pos = block_view.find("@" + clanguml_tag + "{", c_end);
     }
 
-    return res;
+    return {res, util::trim(stripped_comment)};
 };
 
 } // namespace clanguml::decorators

src/decorators/decorators.h

@@ -156,9 +156,10 @@ struct call : public decorator {
  *
  * @param documentation_block Documentation block extracted from source code
  * @param clanguml_tag Name of the clanguml tag (default `uml`)
- * @return List of clang-uml decorators extracted from comment
+ * @return Pair of: a list of clang-uml decorators extracted from comment and
+ *                  a comment stripped of any uml directives
  */
-std::vector<std::shared_ptr<decorator>> parse(
+std::pair<std::vector<std::shared_ptr<decorator>>, std::string> parse(
     std::string documentation_block, const std::string &clanguml_tag = "uml");
 
 } // namespace decorators

src/sequence_diagram/model/message.cc

@@ -59,7 +59,11 @@ const std::string &message::return_type() const { return return_type_; }
 
 const std::optional<std::string> &message::comment() const { return comment_; }
 
-void message::set_comment(std::string c) { comment_ = std::move(c); }
+void message::set_comment(std::string c)
+{
+    if (!c.empty())
+        comment_ = std::move(c);
+}
 
 void message::set_comment(const std::optional<std::string> &c)
 {

src/sequence_diagram/visitor/translation_unit_visitor.cc

@@ -1047,24 +1047,28 @@ bool translation_unit_visitor::VisitCallExpr(clang::CallExpr *expr)
 
     set_source_location(*expr, m);
 
-    process_comment(clanguml::common::get_expression_raw_comment(
+    const auto *raw_expr_comment = clanguml::common::get_expression_raw_comment(
-                        source_manager(), *context().get_ast_context(), expr),
+        source_manager(), *context().get_ast_context(), expr);
-        context().get_ast_context()->getDiagnostics(), m);
+    const auto stripped_comment = process_comment(
+        raw_expr_comment, context().get_ast_context()->getDiagnostics(), m);
 
     if (m.skip())
         return true;
 
     auto generated_message_from_comment = generate_message_from_comment(m);
 
-    if (!generated_message_from_comment && !should_include(expr))
+    if (!generated_message_from_comment && !should_include(expr)) {
+        processed_comments().erase(raw_expr_comment);
         return true;
+    }
 
     if (context().is_expr_in_current_control_statement_condition(expr)) {
         m.set_message_scope(common::model::message_scope_t::kCondition);
     }
 
     if (generated_message_from_comment) {
-        // Do nothing
+        LOG_DBG(
+            "Message for this call expression is taken from comment directive");
     }
     //
     // Call to an overloaded operator
@@ -1148,11 +1152,7 @@ bool translation_unit_visitor::VisitCallExpr(clang::CallExpr *expr)
 
     // Add message to diagram
     if (m.from() > 0 && m.to() > 0) {
-        if (!generated_message_from_comment) {
+        m.set_comment(stripped_comment);
-            auto expr_comment = get_expression_comment(source_manager(),
-                *context().get_ast_context(), context().caller_id(), expr);
-            m.set_comment(expr_comment);
-        }
 
         if (diagram().sequences().find(m.from()) ==
             diagram().sequences().end()) {
@@ -2167,10 +2167,17 @@ std::optional<std::string> translation_unit_visitor::get_expression_comment(
     if (raw_comment == nullptr)
         return {};
 
-    if (!processed_comments_.emplace(caller_id, raw_comment).second) {
+    if (!processed_comments_by_caller_id_.emplace(caller_id, raw_comment)
+             .second) {
         return {};
     }
 
-    return raw_comment->getFormattedText(sm, sm.getDiagnostics());
+    const auto &[decorators, stripped_comment] = decorators::parse(
+        raw_comment->getFormattedText(sm, sm.getDiagnostics()));
+
+    if (stripped_comment.empty())
+        return {};
+
+    return stripped_comment;
 }
 } // namespace clanguml::sequence_diagram::visitor

src/sequence_diagram/visitor/translation_unit_visitor.h

@@ -511,7 +511,7 @@ private:
         already_visited_in_static_declaration_{};
 
     mutable std::set<std::pair<int64_t, const clang::RawComment *>>
-        processed_comments_;
+        processed_comments_by_caller_id_;
 
     template_builder_t template_builder_;
 };

tests/t20048/.clang-uml

@@ -0,0 +1,14 @@
+add_compile_flags:
+  - -fparse-all-comments
+diagrams:
+  t20048_sequence:
+    type: sequence
+    glob:
+      - t20048.cc
+    generate_message_comments: true
+    include:
+      namespaces:
+        - clanguml::t20048
+    using_namespace: clanguml::t20048
+    from:
+      - function: "clanguml::t20048::tmain()"

tests/t20048/t20048.cc

@@ -0,0 +1,40 @@
+#include <future>
+
+namespace clanguml {
+namespace t20048 {
+
+int a1(int x) { return x + 1; }
+
+int a2(int x) { return x + 2; }
+
+int a3(int x) { return x + 3; }
+
+int a4(int x) { return x + 4; }
+
+int a5(int x) { return x + 5; }
+
+int a6(int x) { return x + 6; }
+
+int a7(int x) { return x + 7; }
+
+int tmain()
+{
+    // a1() adds `1` to the result of a2()
+    auto res = a1(a2(a3(0)));
+
+    // This lambda calls a4() which adds `4` to it's argument
+    res = [](auto &&x) { return a4(x); }(0);
+
+    // a5() adds `1` to the result of a6()
+    res = a5(
+        // a6() adds `1` to its argument
+        a6(0));
+
+    // a7() is called via add std::async
+    // \uml{call clanguml::t20048::a7(int)}
+    res = std::async(a7, 10).get();
+
+    return 0;
+}
+}
+}

tests/t20048/test_case.h

@@ -0,0 +1,91 @@
+/**
+ * tests/t20048/test_case.h
+ *
+ * Copyright (c) 2021-2024 Bartek Kryza <bkryza@gmail.com>
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+TEST_CASE("t20048", "[test-case][sequence]")
+{
+    auto [config, db] = load_config("t20048");
+
+    auto diagram = config.diagrams["t20048_sequence"];
+
+    REQUIRE(diagram->name == "t20048_sequence");
+
+    auto model = generate_sequence_diagram(*db, diagram);
+
+    REQUIRE(model->name() == "t20048_sequence");
+
+    {
+        auto src = generate_sequence_puml(diagram, *model);
+        AliasMatcher _A(src);
+
+        REQUIRE_THAT(src, StartsWith("@startuml"));
+        REQUIRE_THAT(src, EndsWith("@enduml\n"));
+
+        // Check if all calls exist
+        REQUIRE_THAT(src, HasCall(_A("tmain()"), _A("a3(int)"), ""));
+        REQUIRE_THAT(src, HasCall(_A("tmain()"), _A("a2(int)"), ""));
+        REQUIRE_THAT(src, HasCall(_A("tmain()"), _A("a1(int)"), ""));
+        REQUIRE_THAT(src, HasCall(_A("tmain()"), _A("a5(int)"), ""));
+        REQUIRE_THAT(src, HasCall(_A("tmain()"), _A("a6(int)"), ""));
+        REQUIRE_THAT(src, HasCall(_A("tmain()"), _A("a7(int)"), ""));
+
+        REQUIRE_THAT(src,
+            HasCall(_A("tmain()"), _A("tmain()::(lambda t20048.cc:26:11)"),
+                "operator()()"));
+        REQUIRE_THAT(src,
+            HasCall(
+                _A("tmain()::(lambda t20048.cc:26:11)"), _A("a4(int)"), ""));
+
+        REQUIRE_THAT(src,
+            HasMessageComment(_A("tmain()"),
+                "a1\\(\\) adds `1` to the result\\n"
+                "of a2\\(\\)"));
+        REQUIRE_THAT(src,
+            HasMessageComment(_A("tmain()"),
+                "This lambda calls a4\\(\\) which\\n"
+                "adds `4` to it's argument"));
+        REQUIRE_THAT(src,
+            HasMessageComment(
+                _A("tmain()"), "a6\\(\\) adds `1` to its argument"));
+        REQUIRE_THAT(src,
+            HasMessageComment(_A("tmain()"),
+                "a5\\(\\) adds `1` to the result\\n"
+                "of a6\\(\\)"));
+        REQUIRE_THAT(src,
+            HasMessageComment(
+                _A("tmain()"), "a7\\(\\) is called via add std::async"));
+
+        save_puml(config.output_directory(), diagram->name + ".puml", src);
+    }
+
+    {
+        auto j = generate_sequence_json(diagram, *model);
+
+        using namespace json;
+
+        save_json(config.output_directory(), diagram->name + ".json", j);
+    }
+
+    {
+        auto src = generate_sequence_mermaid(diagram, *model);
+
+        mermaid::AliasMatcher _A(src);
+        using mermaid::IsClass;
+
+        save_mermaid(config.output_directory(), diagram->name + ".mmd", src);
+    }
+}

tests/test_cases.cc

@@ -474,6 +474,7 @@ using namespace clanguml::test::matchers;
 #include "t20045/test_case.h"
 #include "t20046/test_case.h"
 #include "t20047/test_case.h"
+#include "t20048/test_case.h"
 
 ///
 /// Package diagram tests

tests/test_decorator_parser.cc

@@ -20,6 +20,7 @@
 #include "decorators/decorators.h"
 
 #include "catch.h"
+#include "util/util.h"
 
 TEST_CASE("Test decorator parser on regular comment", "[unit-test]")
 {
@@ -35,9 +36,10 @@ TEST_CASE("Test decorator parser on regular comment", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.empty());
+    CHECK(clanguml::util::trim(comment) == stripped);
 }
 
 TEST_CASE("Test decorator parser on note", "[unit-test]")
@@ -62,7 +64,7 @@ TEST_CASE("Test decorator parser on note", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.size() == 4);
 
@@ -86,6 +88,15 @@ TEST_CASE("Test decorator parser on note", "[unit-test]")
     CHECK(n4);
     CHECK(n4->position == "left");
     CHECK(n4->text == "This is a comment");
+
+    CHECK(stripped == R"(\brief This is a comment.
+
+    This is a longer comment.
+
+    \
+
+    \param a int an int
+    \param b float a float)");
 }
 
 TEST_CASE("Test decorator parser on note with custom tag", "[unit-test]")
@@ -110,7 +121,7 @@ TEST_CASE("Test decorator parser on note with custom tag", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment, "clanguml");
+    auto [decorators, stripped] = parse(comment, "clanguml");
 
     CHECK(decorators.size() == 4);
 
@@ -134,6 +145,15 @@ TEST_CASE("Test decorator parser on note with custom tag", "[unit-test]")
     CHECK(n4);
     CHECK(n4->position == "left");
     CHECK(n4->text == "This is a comment");
+
+    CHECK(stripped == R"(\brief This is a comment.
+
+    This is a longer comment.
+
+    \
+
+    \param a int an int
+    \param b float a float)");
 }
 
 TEST_CASE("Test decorator parser on style", "[unit-test]")
@@ -144,7 +164,7 @@ TEST_CASE("Test decorator parser on style", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.size() == 1);
 
@@ -152,6 +172,7 @@ TEST_CASE("Test decorator parser on style", "[unit-test]")
 
     CHECK(n1);
     CHECK(n1->spec == "#green,dashed,thickness=4");
+    CHECK(stripped.empty());
 }
 
 TEST_CASE("Test decorator parser on aggregation", "[unit-test]")
@@ -162,7 +183,7 @@ TEST_CASE("Test decorator parser on aggregation", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.size() == 1);
 
@@ -170,6 +191,7 @@ TEST_CASE("Test decorator parser on aggregation", "[unit-test]")
 
     CHECK(n1);
     CHECK(n1->multiplicity == "0..1:0..*");
+    CHECK(stripped.empty());
 }
 
 TEST_CASE("Test decorator parser on skip", "[unit-test]")
@@ -180,13 +202,14 @@ TEST_CASE("Test decorator parser on skip", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.size() == 1);
 
     auto n1 = std::dynamic_pointer_cast<skip>(decorators.at(0));
 
     CHECK(n1);
+    CHECK(stripped.empty());
 }
 
 TEST_CASE("Test decorator parser on skiprelationship", "[unit-test]")
@@ -197,13 +220,14 @@ TEST_CASE("Test decorator parser on skiprelationship", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.size() == 1);
 
     auto n1 = std::dynamic_pointer_cast<skip_relationship>(decorators.at(0));
 
     CHECK(n1);
+    CHECK(stripped.empty());
 }
 
 TEST_CASE("Test decorator parser on diagram scope", "[unit-test]")
@@ -215,7 +239,7 @@ TEST_CASE("Test decorator parser on diagram scope", "[unit-test]")
 
     using namespace clanguml::decorators;
 
-    auto decorators = parse(comment);
+    auto [decorators, stripped] = parse(comment);
 
     CHECK(decorators.size() == 1);
 
@@ -232,4 +256,21 @@ TEST_CASE("Test decorator parser on diagram scope", "[unit-test]")
 
     CHECK(n1->applies_to_diagram("diagram2"));
     CHECK(!n1->applies_to_diagram("diagram4"));
+    CHECK(stripped.empty());
+}
+
+TEST_CASE("Test invalid comment - unterminated curly brace", "[unit-test]")
+{
+    std::string comment = R"(
+    Test test test
+    \uml{call clanguml::test:aa()
+    )";
+
+    using namespace clanguml::decorators;
+
+    auto [decorators, stripped] = parse(comment);
+
+    CHECK(decorators.size() == 0);
+
+    CHECK(stripped.empty());
 }