Add generic AutoPopStack class #289

ghost · 2020-05-05T12:35:57Z

Generic autopop stack class is intended to be used in code base which uses stack-based algorithms like symbol or node tracking, e.g. SyntaxTreeContext.

fangism · 2020-05-05T16:52:22Z

common/util/auto_pop_stack.h

+class AutoPopStack {
+ public:
+  typedef T value_type;
+  typedef typename std::remove_pointer<value_type>::type basic_value_type;


Let's keep the interface so that this class can work with pointers and non-pointers alike. In other words, let function parameters and parameters use value_type and T& for reference_type, const T& for const_reference_type (the same way most STL containers are designed).

So no auto-deferencing/address-taking at the interface.
This means that the user of this class will need to pass pointers instead of references.

Done & done

fangism · 2020-05-05T16:53:55Z

common/util/auto_pop_stack.h

+      stack_ = stack;
+      stack->Push(std::move(value));
+    }
+    AutoPop(this_type* stack, basic_value_type& value) {


Related to above comment, this should support const value_type& (copy).

fangism · 2020-05-05T16:55:42Z

common/util/auto_pop_stack.h

+  typedef typename std::remove_pointer<value_type>::type basic_value_type;
+  typedef AutoPopStack<value_type> this_type;
+
+  // member class to handle push and pop of stack safely


Also emphasize that this is the only public interface that modifies the number of elements in the stack.

fangism · 2020-05-05T17:02:42Z

common/util/auto_pop_stack.h

+  // All elements of stack are non-null because they refer to traversed nodes.
+  typedef std::vector<value_type> stack_type;
+  typedef typename stack_type::iterator iterator;
+  typedef typename stack_type::const_iterator const_iterator;
+  typedef typename stack_type::const_reverse_iterator const_reverse_iterator;


Move these typedefs near top of class.

fangism · 2020-05-05T17:04:19Z

common/util/auto_pop_stack.h

+    return const_cast<value_type&>(
+        static_cast<const this_type&>(*this).top());
+  }


No need for casting, just return stack_.back();. This should internally look identical to the const method counterpart.

Just wanted to reuse const top() method. Fixed.

fangism · 2020-05-05T17:04:41Z

common/util/auto_pop_stack.h

+  }
+
+  // Push a value_type onto the stack (takes reference)
+  void Push(const basic_value_type& value) {


const value_type&

common/util/auto_pop_stack.h

fangism

Only minor things remain. Looks good.

fangism · 2020-05-05T20:08:33Z

common/text/syntax_tree_context.h


 protected:
-  // Pop the top SyntaxTreeNode off of the stack
-  void Pop();
-
-  // Push a SyntaxTreeNode onto the stack (takes address)
-  void Push(const SyntaxTreeNode& node);
-
  // Stack of ancestors of the current node that is updated as the tree
  // is traversed. Top of the stack is closest ancestor.
  // A vector is chosen to allow random access and searches from either end of
  // the stack.
-  stack_type stack_;
+  // private:
+  //  base_type::stack_type base_type::stack_;


Can delete this section now.

fangism · 2020-05-05T20:09:46Z

common/util/auto_pop_stack.h

+#include <iterator>
+#include <type_traits>
+#include <vector>
+#include "common/util/iterator_adaptors.h"


minor nit: blank line above here to separate groups of headers

fangism · 2020-05-05T20:15:38Z

common/util/auto_pop_stack.h

+
+namespace verible {
+
+// AutoPop stack class. Templated version of SyntaxTreeContext::AutoPop


Update comment to not mention SyntaxTreeContext::AutoPop (reference is cyclic).
Suggest describing use cases of this class, and a motivating example.
Also worth mentioning that this provides public read-only random access/iteration into the stack elements.

fangism · 2020-05-05T20:16:55Z

common/util/auto_pop_stack.h

+  typedef AutoPopStack<value_type> this_type;
+
+  typedef value_type& reference_type;
+  typedef const value_type& const_reference_type;


Let's follow STL convention and call this const_reference.

Also fixed reference

fangism · 2020-05-05T20:17:26Z

common/util/auto_pop_stack.h

+#ifndef VERIBLE_COMMON_UTIL_AUTO_POP_STACK_H_
+#define VERIBLE_COMMON_UTIL_AUTO_POP_STACK_H_
+
+#include <iterator>


add (for size_t):

#include

Added cstddef. Hope it's that what you meant.

fangism · 2020-05-05T20:21:17Z

common/text/syntax_tree_context.h

@@ -31,38 +33,16 @@ namespace verible {
 // Note: Public methods are named to follow STL convention for std::stack.
 // TODO(fangism): implement a general ForwardMatcher and ReverseMatcher
 // interface that can express AND/OR/NOT.
-class SyntaxTreeContext {
+class SyntaxTreeContext : public AutoPopStack<const SyntaxTreeNode*> {


Document that one of the invariants is that elements of the (internal) stack are restricted to non-nullptrs. (This was the case before refactoring.)

fangism · 2020-05-05T20:26:11Z

common/text/syntax_tree_context.h

-  const_reverse_iterator rbegin() const { return stack_.rbegin(); }
-  const_reverse_iterator rend() const { return stack_.rend(); }
+  const SyntaxTreeNode& top() const {
+    return *ABSL_DIE_IF_NULL(base_type::top());


You can hide the base class's interface with:

protected: using base_type::top;

This makes it clear that you are further restricting direct uses of the base class methods intentionally, and here is where you can alter the interface to return references if you wish.

Optionally, you can also be explicit about what base class interfaces you wish to expose:

public: using base_type::begin;

Just restricted access to base_type::top. Also removed base_type:: from methods like rbegin, rend and so on.

Signed-off-by: Lukasz Dalek <[email protected]>

fangism · 2020-05-06T15:07:09Z

common/util/auto_pop_stack.h

+#ifndef VERIBLE_COMMON_UTIL_AUTO_POP_STACK_H_
+#define VERIBLE_COMMON_UTIL_AUTO_POP_STACK_H_
+
+#include <iterator>


issues #289 PiperOrigin-RevId: 310247369

googlebot added the cla: yes All contributors in pull request have signed the CLA with Google. label May 5, 2020

ghost mentioned this pull request May 5, 2020

WIP: Add "forbid-implicit-declarations" lint rule #244

Draft

ghost requested a review from fangism May 5, 2020 12:39

fangism requested changes May 5, 2020

View reviewed changes

ghost force-pushed the autopop branch from 723534d to b2fb535 Compare May 5, 2020 20:00

fangism requested changes May 5, 2020

View reviewed changes

Lukasz Dalek added 2 commits May 6, 2020 13:50

Add generic AutoPop stack class

650cef0

Signed-off-by: Lukasz Dalek <[email protected]>

SyntaxTreeContext: Use generic AutoPopStack class

4aa1261

Signed-off-by: Lukasz Dalek <[email protected]>

ghost force-pushed the autopop branch from b2fb535 to 4aa1261 Compare May 6, 2020 11:55

fangism approved these changes May 6, 2020

View reviewed changes

hzeller closed this in b55e90a May 6, 2020

hzeller pushed a commit that referenced this pull request May 6, 2020

Add unit-tests for AutoPopStack.

8bfe7a7

issues #289 PiperOrigin-RevId: 310247369

nikhiljha pushed a commit to nikhiljha/verible that referenced this pull request Sep 27, 2022

installers/npm: use global style (chipsalliance#289)

a5168bb

nikhiljha pushed a commit to nikhiljha/verible that referenced this pull request Sep 27, 2022

fix(julia-lsp): adjust for changes in latest release (chipsalliance#289)

cb7bdcd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic AutoPopStack class #289

Add generic AutoPopStack class #289

ghost commented May 5, 2020

fangism May 5, 2020

ghost May 5, 2020

fangism May 5, 2020

ghost May 5, 2020

fangism May 5, 2020

ghost May 5, 2020

fangism May 5, 2020

ghost May 5, 2020

fangism May 5, 2020

ghost May 5, 2020

fangism May 5, 2020

ghost May 5, 2020

fangism left a comment

fangism May 5, 2020

ghost May 6, 2020

fangism May 5, 2020

ghost May 6, 2020

fangism May 5, 2020

ghost May 6, 2020

fangism May 5, 2020

ghost May 6, 2020

fangism May 5, 2020

ghost May 6, 2020

fangism May 6, 2020

fangism May 5, 2020

ghost May 6, 2020

fangism May 5, 2020

ghost May 6, 2020

fangism May 6, 2020


		namespace verible {

		// AutoPop stack class. Templated version of SyntaxTreeContext::AutoPop

Add generic AutoPopStack class #289

Add generic AutoPopStack class #289

Conversation

ghost commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment