From cc575dd2cefce3170655a026dbf058a42e1a4330 Mon Sep 17 00:00:00 2001 From: peter klausler Date: Tue, 10 Nov 2020 14:56:51 -0800 Subject: [PATCH] [flang][MSVC] Use list rather than forward_list<> in Messages The implementation of Messages with forward_list<> makes some nonstandard assumptions about the validity of iterators that don't hold up with MSVC's implementation. Use list<> instead. The measured performance is comparable. This change obviated a distinction between two member functions of Messages, and the uses of one have been replaced with calls to the other. Similar usage in CharBuffer was also replaced for consistency. Differential revision: https://reviews.llvm.org/D91210 --- flang/include/flang/Parser/char-buffer.h | 11 +++---- flang/include/flang/Parser/instrumented-parser.h | 2 +- flang/include/flang/Parser/message.h | 37 +++++------------------- flang/lib/Parser/basic-parsers.h | 6 ++-- flang/lib/Parser/char-buffer.cpp | 5 ++-- flang/lib/Parser/message.cpp | 13 +++------ 6 files changed, 21 insertions(+), 53 deletions(-) diff --git a/flang/include/flang/Parser/char-buffer.h b/flang/include/flang/Parser/char-buffer.h index 1879e19..9d3812c 100644 --- a/flang/include/flang/Parser/char-buffer.h +++ b/flang/include/flang/Parser/char-buffer.h @@ -13,7 +13,7 @@ // a stream of bytes. #include -#include +#include #include #include #include @@ -24,13 +24,12 @@ class CharBuffer { public: CharBuffer() {} CharBuffer(CharBuffer &&that) - : blocks_(std::move(that.blocks_)), last_{that.last_}, - bytes_{that.bytes_}, lastBlockEmpty_{that.lastBlockEmpty_} { + : blocks_(std::move(that.blocks_)), bytes_{that.bytes_}, + lastBlockEmpty_{that.lastBlockEmpty_} { that.clear(); } CharBuffer &operator=(CharBuffer &&that) { blocks_ = std::move(that.blocks_); - last_ = that.last_; bytes_ = that.bytes_; lastBlockEmpty_ = that.lastBlockEmpty_; that.clear(); @@ -42,7 +41,6 @@ public: void clear() { blocks_.clear(); - last_ = blocks_.end(); bytes_ = 0; lastBlockEmpty_ = false; } @@ -65,8 +63,7 @@ private: }; int LastBlockOffset() const { return bytes_ % Block::capacity; } - std::forward_list blocks_; - std::forward_list::iterator last_{blocks_.end()}; + std::list blocks_; std::size_t bytes_{0}; bool lastBlockEmpty_{false}; }; diff --git a/flang/include/flang/Parser/instrumented-parser.h b/flang/include/flang/Parser/instrumented-parser.h index 1bc1c52..9d958b9 100644 --- a/flang/include/flang/Parser/instrumented-parser.h +++ b/flang/include/flang/Parser/instrumented-parser.h @@ -63,7 +63,7 @@ public: Messages messages{std::move(state.messages())}; std::optional result{parser_.Parse(state)}; log->Note(at, tag_, result.has_value(), state); - state.messages().Restore(std::move(messages)); + state.messages().Annex(std::move(messages)); return result; } } diff --git a/flang/include/flang/Parser/message.h b/flang/include/flang/Parser/message.h index cbb42df..8286384 100644 --- a/flang/include/flang/Parser/message.h +++ b/flang/include/flang/Parser/message.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -213,43 +214,22 @@ private: class Messages { public: Messages() {} - Messages(Messages &&that) : messages_{std::move(that.messages_)} { - if (!messages_.empty()) { - last_ = that.last_; - that.ResetLastPointer(); - } - } + Messages(Messages &&that) : messages_{std::move(that.messages_)} {} Messages &operator=(Messages &&that) { messages_ = std::move(that.messages_); - if (messages_.empty()) { - ResetLastPointer(); - } else { - last_ = that.last_; - that.ResetLastPointer(); - } return *this; } - std::forward_list &messages() { return messages_; } + std::list &messages() { return messages_; } bool empty() const { return messages_.empty(); } - void clear(); + void clear() { messages_.clear(); } template Message &Say(A &&...args) { - last_ = messages_.emplace_after(last_, std::forward(args)...); - return *last_; + return messages_.emplace_back(std::forward(args)...); } void Annex(Messages &&that) { - if (!that.messages_.empty()) { - messages_.splice_after(last_, that.messages_); - last_ = that.last_; - that.ResetLastPointer(); - } - } - - void Restore(Messages &&that) { - that.Annex(std::move(*this)); - *this = std::move(that); + messages_.splice(messages_.end(), that.messages_); } bool Merge(const Message &); @@ -262,10 +242,7 @@ public: bool AnyFatalError() const; private: - void ResetLastPointer() { last_ = messages_.before_begin(); } - - std::forward_list messages_; - std::forward_list::iterator last_{messages_.before_begin()}; + std::list messages_; }; class ContextualMessages { diff --git a/flang/lib/Parser/basic-parsers.h b/flang/lib/Parser/basic-parsers.h index 7f349d7..03238b5 100644 --- a/flang/lib/Parser/basic-parsers.h +++ b/flang/lib/Parser/basic-parsers.h @@ -108,7 +108,7 @@ public: ParseState backtrack{state}; std::optional result{parser_.Parse(state)}; if (result) { - state.messages().Restore(std::move(messages)); + state.messages().Annex(std::move(messages)); } else { state = std::move(backtrack); state.messages() = std::move(messages); @@ -311,7 +311,7 @@ public: ParseRest<1>(result, state, backtrack); } } - state.messages().Restore(std::move(messages)); + state.messages().Annex(std::move(messages)); return result; } @@ -371,7 +371,7 @@ public: } Messages messages{std::move(state.messages())}; if (std::optional ax{pa_.Parse(state)}) { - state.messages().Restore(std::move(messages)); + state.messages().Annex(std::move(messages)); return ax; } messages.Annex(std::move(state.messages())); diff --git a/flang/lib/Parser/char-buffer.cpp b/flang/lib/Parser/char-buffer.cpp index 780d7e8..0649d2c 100644 --- a/flang/lib/Parser/char-buffer.cpp +++ b/flang/lib/Parser/char-buffer.cpp @@ -18,14 +18,13 @@ char *CharBuffer::FreeSpace(std::size_t &n) { int offset{LastBlockOffset()}; if (blocks_.empty()) { blocks_.emplace_front(); - last_ = blocks_.begin(); lastBlockEmpty_ = true; } else if (offset == 0 && !lastBlockEmpty_) { - last_ = blocks_.emplace_after(last_); + blocks_.emplace_back(); lastBlockEmpty_ = true; } n = Block::capacity - offset; - return last_->data + offset; + return blocks_.back().data + offset; } void CharBuffer::Claim(std::size_t n) { diff --git a/flang/lib/Parser/message.cpp b/flang/lib/Parser/message.cpp index 6819ee4d..bef55f3 100644 --- a/flang/lib/Parser/message.cpp +++ b/flang/lib/Parser/message.cpp @@ -260,11 +260,6 @@ bool Message::AtSameLocation(const Message &that) const { location_, that.location_); } -void Messages::clear() { - messages_.clear(); - ResetLastPointer(); -} - bool Messages::Merge(const Message &msg) { if (msg.IsMergeable()) { for (auto &m : messages_) { @@ -284,12 +279,12 @@ void Messages::Merge(Messages &&that) { if (Merge(that.messages_.front())) { that.messages_.pop_front(); } else { - messages_.splice_after( - last_, that.messages_, that.messages_.before_begin()); - ++last_; + auto next{that.messages_.begin()}; + ++next; + messages_.splice( + messages_.end(), that.messages_, that.messages_.begin(), next); } } - that.ResetLastPointer(); } } -- 2.7.4