From 7c30ba7c464e44dc38d262ecc43885141bfb6e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20M=C3=BCller?= Date: Tue, 10 Oct 2017 17:07:41 +0200 Subject: [PATCH 1/4] [breaking] Ensure matching line numbers after preprocessing Unmatched documentation comments are not just a string, but also contain the line number as well. --- include/cppast/cpp_file.hpp | 28 +++++++++++++++-------- src/libclang/libclang_parser.cpp | 2 +- src/libclang/preprocessor.cpp | 39 ++++++++++++++++++++++++++++---- test/cpp_preprocessor.cpp | 39 +++++++++++++++++++++++++++++++- 4 files changed, 91 insertions(+), 17 deletions(-) diff --git a/include/cppast/cpp_file.hpp b/include/cppast/cpp_file.hpp index b45ebec..942e4f5 100644 --- a/include/cppast/cpp_file.hpp +++ b/include/cppast/cpp_file.hpp @@ -13,6 +13,18 @@ namespace cppast { + /// An unmatched documentation comment. + struct cpp_doc_comment + { + std::string content; + unsigned line; + + cpp_doc_comment(std::string content, unsigned line) + : content(std::move(content)), line(line) + { + } + }; + /// A [cppast::cpp_entity]() modelling a file. /// /// This is the top-level entity of the AST. @@ -26,9 +38,7 @@ namespace cppast { public: /// \effects Sets the file name. - explicit builder(std::string name) : file_(new cpp_file(std::move(name))) - { - } + explicit builder(std::string name) : file_(new cpp_file(std::move(name))) {} /// \effects Adds an entity. void add_child(std::unique_ptr child) noexcept @@ -37,9 +47,9 @@ namespace cppast } /// \effects Adds an unmatched documentation comment. - void add_unmatched_comment(std::string str) + void add_unmatched_comment(cpp_doc_comment comment) { - file_->comments_.push_back(std::move(str)); + file_->comments_.push_back(std::move(comment)); } /// \returns The not yet finished file. @@ -62,20 +72,18 @@ namespace cppast }; /// \returns The unmatched documentation comments. - type_safe::array_ref unmatched_comments() const noexcept + type_safe::array_ref unmatched_comments() const noexcept { return type_safe::ref(comments_.data(), comments_.size()); } private: - cpp_file(std::string name) : cpp_entity(std::move(name)) - { - } + cpp_file(std::string name) : cpp_entity(std::move(name)) {} /// \returns [cpp_entity_type::file_t](). cpp_entity_kind do_get_entity_kind() const noexcept override; - std::vector comments_; + std::vector comments_; }; /// \exclude diff --git a/src/libclang/libclang_parser.cpp b/src/libclang/libclang_parser.cpp index 4fcae1e..996b8ec 100644 --- a/src/libclang/libclang_parser.cpp +++ b/src/libclang/libclang_parser.cpp @@ -510,7 +510,7 @@ std::unique_ptr libclang_parser::do_parse(const cpp_entity_index& idx, for (auto& c : preprocessed.comments) { if (!c.comment.empty()) - builder.add_unmatched_comment(std::move(c.comment)); + builder.add_unmatched_comment(cpp_doc_comment(std::move(c.comment), c.line)); } if (context.error) diff --git a/src/libclang/preprocessor.cpp b/src/libclang/preprocessor.cpp index 45c3d52..3de001c 100644 --- a/src/libclang/preprocessor.cpp +++ b/src/libclang/preprocessor.cpp @@ -191,6 +191,16 @@ namespace { } + void undo_write() + { + if (write_ == true && !result_->empty()) + { + if (result_->back() == '\n') + --cur_line_; + result_->pop_back(); + } + } + void write_str(std::string str) { if (write_ == false) @@ -363,7 +373,7 @@ namespace break; } } - p.skip(); + skip(p, "\n"); return result; } @@ -734,16 +744,32 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co switch (lm.value().flag) { case linemarker::line_directive: - break; // ignore - // no need to handle it, preprocessed output doesn't need to match line numbers precisely + if (file_depth == 0u && lm.value().line > p.cur_line()) + { + // write the necessary newlines + p.write_str(std::string(lm.value().line - p.cur_line(), '\n')); + DEBUG_ASSERT(p.cur_line() == lm.value().line, detail::assert_handler{}); + } + else if (file_depth == 0u) + // current line may be one higher than the expected line, + // because the include directives inserted by -dI have an additional newline + // that is not yet removed + DEBUG_ASSERT(p.cur_line() - lm.value().line <= 1u, detail::assert_handler{}); + break; case linemarker::enter_new: if (file_depth == 0u && lm.value().file.front() != '<') { // this file is directly included by the given file - // write include with full path - // note: don't build include here, do it when an #include is encountered + // and it is not a fake file like builtin or command line + + // remove the trailing newline from the include that brought us here + DEBUG_ASSERT(p.was_newl(), detail::assert_handler{}); + p.undo_write(); + + // and write include with full path p.write_str("#include \"" + lm.value().file + "\"\n"); + // note: don't build include here, do it when an #include is encountered } ++file_depth; @@ -755,6 +781,9 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co if (file_depth == 0u) { DEBUG_ASSERT(lm.value().file == path, detail::assert_handler{}); + // difference is 1 if coming from an included file (because newline of include is already written) + // difference is 0 if coming from a builtin file (because no include has been written) + DEBUG_ASSERT(p.cur_line() - lm.value().line <= 1u, detail::assert_handler{}); p.enable_write(); } break; diff --git a/test/cpp_preprocessor.cpp b/test/cpp_preprocessor.cpp index d2db720..316906c 100644 --- a/test/cpp_preprocessor.cpp +++ b/test/cpp_preprocessor.cpp @@ -136,6 +136,43 @@ TEST_CASE("cpp_include_directive", "[!hide][clang4]") REQUIRE(count == 1u); } +TEST_CASE("preprocessor line numbers") +{ + auto code = R"(/// 1 + +#include + +/// 5 + +#include + +int foo; + +/// 11 + +#define foo \ + \ + \ + int \ + main() + +/// 19 + +foo {} + +/// 23 + +#include + +/// 27 +)"; + + auto file = parse({}, "preprocessor_line_numbers.cpp", code); + for (auto& comment : file->unmatched_comments()) + REQUIRE(comment.line == std::stoi(comment.content)); + REQUIRE((file->unmatched_comments().size() == 6u)); +} + TEST_CASE("comment matching") { auto code = R"( @@ -205,6 +242,6 @@ void j(); }); for (auto& comment : file->unmatched_comments()) - REQUIRE(comment == "u"); + REQUIRE(comment.content == "u"); REQUIRE((file->unmatched_comments().size() == 3u)); } From d088b44db648bc3317d2647226485d26e42a0faf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20M=C3=BCller?= Date: Tue, 10 Oct 2017 17:16:10 +0200 Subject: [PATCH 2/4] Add line numbers to diagnostics --- src/libclang/parse_error.hpp | 8 +++++++- src/libclang/preprocessor.cpp | 9 ++++++--- src/libclang/type_parser.cpp | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/libclang/parse_error.hpp b/src/libclang/parse_error.hpp index da34438..bb40f2d 100644 --- a/src/libclang/parse_error.hpp +++ b/src/libclang/parse_error.hpp @@ -19,7 +19,13 @@ namespace cppast { inline source_location make_location(const CXCursor& cur) { - return source_location::make_entity(get_display_name(cur).c_str()); + auto loc = clang_getCursorLocation(cur); + + CXString file; + unsigned line; + clang_getPresumedLocation(loc, &file, &line, nullptr); + + return source_location::make_file(cxstring(file).c_str(), line); } inline source_location make_location(const CXType& type) diff --git a/src/libclang/preprocessor.cpp b/src/libclang/preprocessor.cpp index 3de001c..aa75277 100644 --- a/src/libclang/preprocessor.cpp +++ b/src/libclang/preprocessor.cpp @@ -701,7 +701,8 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co { auto message = detail::format("parsing macro '", macro->name(), "'"); logger.log("preprocessor", - diagnostic{std::move(message), source_location::make_file(path), + diagnostic{std::move(message), + source_location::make_file(path, p.cur_line()), severity::debug}); } @@ -715,7 +716,8 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co { auto message = detail::format("undefining macro '", undef.value(), "'"); logger.log("preprocessor", - diagnostic{std::move(message), source_location::make_file(path), + diagnostic{std::move(message), + source_location::make_file(path, p.cur_line()), severity::debug}); } result.macros.erase(std::remove_if(result.macros.begin(), result.macros.end(), @@ -732,7 +734,8 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co auto message = detail::format("parsing include '", include.value().file.name(), "'"); logger.log("preprocessor", - diagnostic{std::move(message), source_location::make_file(path), + diagnostic{std::move(message), + source_location::make_file(path, p.cur_line()), severity::debug}); } result.includes.push_back(std::move(include.value())); diff --git a/src/libclang/type_parser.cpp b/src/libclang/type_parser.cpp index 827d8ed..a96332d 100644 --- a/src/libclang/type_parser.cpp +++ b/src/libclang/type_parser.cpp @@ -553,7 +553,7 @@ namespace { auto msg = detail::format("unexpected type of kind '", detail::get_type_kind_spelling(type).c_str(), "'"); - auto location = source_location::make_entity(get_type_spelling(type).c_str()); + auto location = detail::make_location(type); context.logger->log("libclang parser", diagnostic{msg, location, severity::warning}); } // fallthrough From b28fff6c371089223598b13f1276db224f085bc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20M=C3=BCller?= Date: Tue, 10 Oct 2017 21:13:29 +0200 Subject: [PATCH 3/4] Fix preprocessor line numbers if includes aren't supported --- src/libclang/libclang_parser.cpp | 26 ++++++++++++++------------ src/libclang/preprocessor.cpp | 20 ++++++++++++++++---- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/libclang/libclang_parser.cpp b/src/libclang/libclang_parser.cpp index 996b8ec..12b602a 100644 --- a/src/libclang/libclang_parser.cpp +++ b/src/libclang/libclang_parser.cpp @@ -403,9 +403,8 @@ namespace auto args = get_arguments(config); CXTranslationUnit tu; - auto flags = CXTranslationUnit_Incomplete | CXTranslationUnit_KeepGoing; - if (detail::libclang_compile_config_access::clang_version(config) >= 40000) - flags |= CXTranslationUnit_DetailedPreprocessingRecord; + auto flags = CXTranslationUnit_Incomplete | CXTranslationUnit_KeepGoing + | CXTranslationUnit_DetailedPreprocessingRecord; auto error = clang_parseTranslationUnit2(idx.get(), path, // index and path @@ -479,17 +478,20 @@ std::unique_ptr libclang_parser::do_parse(const cpp_entity_index& idx, detail::visit_tu(tu, path.c_str(), [&](const CXCursor& cur) { if (clang_getCursorKind(cur) == CXCursor_InclusionDirective) { - DEBUG_ASSERT(include_iter != preprocessed.includes.end() - && get_line_no(cur) >= include_iter->line, - detail::assert_handler{}); + if (!preprocessed.includes.empty()) + { + DEBUG_ASSERT(include_iter != preprocessed.includes.end() + && get_line_no(cur) >= include_iter->line, + detail::assert_handler{}); - auto include = - cpp_include_directive::build(std::move(include_iter->file), include_iter->kind, - detail::get_cursor_name(cur).c_str()); - context.comments.match(*include, include_iter->line); - builder.add_child(std::move(include)); + auto include = + cpp_include_directive::build(std::move(include_iter->file), include_iter->kind, + detail::get_cursor_name(cur).c_str()); + context.comments.match(*include, include_iter->line); + builder.add_child(std::move(include)); - ++include_iter; + ++include_iter; + } } else if (clang_getCursorKind(cur) != CXCursor_MacroDefinition) { diff --git a/src/libclang/preprocessor.cpp b/src/libclang/preprocessor.cpp index aa75277..c6802fd 100644 --- a/src/libclang/preprocessor.cpp +++ b/src/libclang/preprocessor.cpp @@ -30,6 +30,11 @@ namespace return '"' + std::move(str) + '"'; } + bool support_include(const libclang_compile_config& c) + { + return detail::libclang_compile_config_access::clang_version(c) >= 40000; + } + // build the command that runs the preprocessor std::string get_command(const libclang_compile_config& c, const char* full_path) { @@ -39,7 +44,7 @@ namespace // -C: keep comments // -dD: print macro definitions as well auto flags = std::string("-x c++ -I. -E -C -dD"); - if (detail::libclang_compile_config_access::clang_version(c) >= 40000) + if (support_include(c)) // -Xclang -dI: print include directives as well (clang >= 4.0.0) flags += " -Xclang -dI"; // -fno-caret-diagnostics: don't show the source extract in diagnostics @@ -784,9 +789,16 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co if (file_depth == 0u) { DEBUG_ASSERT(lm.value().file == path, detail::assert_handler{}); - // difference is 1 if coming from an included file (because newline of include is already written) - // difference is 0 if coming from a builtin file (because no include has been written) - DEBUG_ASSERT(p.cur_line() - lm.value().line <= 1u, detail::assert_handler{}); + + if (lm.value().line > p.cur_line()) + // might happen in weird cases + p.write_str(std::string(lm.value().line - p.cur_line(), '\n')); + else + // difference is 1 if coming from an included file (because newline of include is already written) + // difference is 0 if coming from a builtin file (because no include has been written) + DEBUG_ASSERT(p.cur_line() - lm.value().line <= 1u, + detail::assert_handler{}); + p.enable_write(); } break; From 845ef6101725d3d408bfb41fc04e28d028b5e151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20M=C3=BCller?= Date: Tue, 10 Oct 2017 22:07:36 +0200 Subject: [PATCH 4/4] Use #line directive to enforce matching line numbers clang preprocessing output is different on Windows for some reason... --- src/libclang/parse_functions.cpp | 2 +- src/libclang/preprocessor.cpp | 40 ++++++-------------------------- 2 files changed, 8 insertions(+), 34 deletions(-) diff --git a/src/libclang/parse_functions.cpp b/src/libclang/parse_functions.cpp index c001d51..a47fc15 100644 --- a/src/libclang/parse_functions.cpp +++ b/src/libclang/parse_functions.cpp @@ -80,7 +80,7 @@ void detail::comment_context::match(cpp_entity& e, const CXCursor& cur) const { auto pos = clang_getRangeStart(clang_getCursorExtent(cur)); unsigned line; - clang_getSpellingLocation(pos, nullptr, &line, nullptr, nullptr); + clang_getPresumedLocation(pos, nullptr, &line, nullptr); match(e, line); } diff --git a/src/libclang/preprocessor.cpp b/src/libclang/preprocessor.cpp index c6802fd..15494f0 100644 --- a/src/libclang/preprocessor.cpp +++ b/src/libclang/preprocessor.cpp @@ -196,14 +196,10 @@ namespace { } - void undo_write() + void set_line(unsigned line) { - if (write_ == true && !result_->empty()) - { - if (result_->back() == '\n') - --cur_line_; - result_->pop_back(); - } + *result_ += "#line " + std::to_string(line) + "\n"; + cur_line_ = line; } void write_str(std::string str) @@ -752,17 +748,8 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co switch (lm.value().flag) { case linemarker::line_directive: - if (file_depth == 0u && lm.value().line > p.cur_line()) - { - // write the necessary newlines - p.write_str(std::string(lm.value().line - p.cur_line(), '\n')); - DEBUG_ASSERT(p.cur_line() == lm.value().line, detail::assert_handler{}); - } - else if (file_depth == 0u) - // current line may be one higher than the expected line, - // because the include directives inserted by -dI have an additional newline - // that is not yet removed - DEBUG_ASSERT(p.cur_line() - lm.value().line <= 1u, detail::assert_handler{}); + if (file_depth == 0u) + p.set_line(lm.value().line); break; case linemarker::enter_new: @@ -771,11 +758,7 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co // this file is directly included by the given file // and it is not a fake file like builtin or command line - // remove the trailing newline from the include that brought us here - DEBUG_ASSERT(p.was_newl(), detail::assert_handler{}); - p.undo_write(); - - // and write include with full path + // write include with full path p.write_str("#include \"" + lm.value().file + "\"\n"); // note: don't build include here, do it when an #include is encountered } @@ -789,16 +772,7 @@ detail::preprocessor_output detail::preprocess(const libclang_compile_config& co if (file_depth == 0u) { DEBUG_ASSERT(lm.value().file == path, detail::assert_handler{}); - - if (lm.value().line > p.cur_line()) - // might happen in weird cases - p.write_str(std::string(lm.value().line - p.cur_line(), '\n')); - else - // difference is 1 if coming from an included file (because newline of include is already written) - // difference is 0 if coming from a builtin file (because no include has been written) - DEBUG_ASSERT(p.cur_line() - lm.value().line <= 1u, - detail::assert_handler{}); - + p.set_line(lm.value().line); p.enable_write(); } break;