Re: [PATCH] make rich_location safe to copy

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On 6/16/21 10:35 AM, Jason Merrill wrote:
> On 6/16/21 12:11 PM, Martin Sebor wrote:
>> On 6/16/21 9:06 AM, David Malcolm wrote:
>>> On Wed, 2021-06-16 at 08:52 -0600, Martin Sebor wrote:
>>>> On 6/16/21 6:38 AM, David Malcolm wrote:
>>>>> On Tue, 2021-06-15 at 19:48 -0600, Martin Sebor wrote:
>>>>>
>>>>> Thanks for writing the patch.
>>>>>
>>>>>> While debugging locations I noticed the semi_embedded_vec template
>>>>>> in line-map.h doesn't declare a copy ctor or copy assignment, but
>>>>>> is being copied in a couple of places in the C++ parser (via
>>>>>> gcc_rich_location).  It gets away with it most likely because it
>>>>>> never grows beyond the embedded buffer.
>>>>>
>>>>> Where are these places?  I wasn't aware of this.
>>>>
>>>> They're in the attached file along with the diff to reproduce
>>>> the errors.
>>>
>>> Thanks.
>>>
>>> Looks like:
>>>
>>>     gcc_rich_location richloc = tok->location;
>>>
>>> is implicitly constructing a gcc_rich_location, then copying it to
>>> richloc.  This should instead be simply:
>>>
>>>     gcc_rich_location richloc (tok->location);
>>>
>>> which directly constructs the richloc in place, as I understand it.
>>
>> I see, tok->location is location_t here, and the gcc_rich_location
>> ctor that takes it is not declared explicit (that would prevent
>> the implicit conversion).
>>
>> The attached patch solves the rich_location problem by a) making
>> the ctor explicit, b) disabling the rich_location copy ctor, c)
>> changing the parser to use direct initialization.  (I CC Jason
>> as a heads up on the C++ FE bits.)
> 
> The C++ bits are fine.
> 
>> The semi_embedded_vec should be fixed as well, regardless of this
>> particular use and patch.  Let me know if it's okay to commit (I'm
>> not open to disabling its copy ctor).
> 
>> +  /* Not copyable or assignable.  */
> 
> This comment needs a rationale.

Done in the attached patch.

Providing a rationale in each instance sounds like a good addition
to the coding conventions.  Let me propose a patch for that.

Martin

[-- Attachment #2: gcc-rich-location.diff --]
[-- Type: text/x-patch, Size: 2737 bytes --]

gcc/cp/ChangeLog:

	* parser.c (cp_parser_selection_statement): Use direct initialization
	instead of copy.

gcc/ChangeLog:

	* gcc-rich-location.h (gcc_rich_location): Make ctor explicit.

libcpp/ChangeLog:

	* include/line-map.h (class rich_location): Disable copying and
	assignment.

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d57ddc4560d..848e4823258 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -12355,7 +12355,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 	    IF_STMT_CONSTEVAL_P (statement) = true;
 	    condition = finish_if_stmt_cond (boolean_false_node, statement);
 
-	    gcc_rich_location richloc = tok->location;
+	    gcc_rich_location richloc (tok->location);
 	    bool non_compound_stmt_p = false;
 	    if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
 	      {
@@ -12383,7 +12383,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 						RID_ELSE))
 	      {
 		cp_token *else_tok = cp_lexer_peek_token (parser->lexer);
-		gcc_rich_location else_richloc = else_tok->location;
+		gcc_rich_location else_richloc (else_tok->location);
 		guard_tinfo = get_token_indent_info (else_tok);
 		/* Consume the `else' keyword.  */
 		cp_lexer_consume_token (parser->lexer);
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index 00747631025..2a9e5db65d5 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -21,14 +21,16 @@ along with GCC; see the file COPYING3.  If not see
 #define GCC_RICH_LOCATION_H
 
 /* A gcc_rich_location is libcpp's rich_location with additional
-   helper methods for working with gcc's types.  */
+   helper methods for working with gcc's types.  The class is not
+   copyable or assignable because rich_location isn't. */
+
 class gcc_rich_location : public rich_location
 {
  public:
   /* Constructors.  */
 
   /* Constructing from a location.  */
-  gcc_rich_location (location_t loc, const range_label *label = NULL)
+  explicit gcc_rich_location (location_t loc, const range_label *label = NULL)
   : rich_location (line_table, loc, label)
   {
   }
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 7d964172469..464494bfb5b 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1670,6 +1670,12 @@ class rich_location
   /* Destructor.  */
   ~rich_location ();
 
+  /* The class manages the memory pointed to by the elements of
+     the M_FIXIT_HINTS vector and is not meant to be copied or
+     assigned.  */
+  rich_location (const rich_location &) = delete;
+  void operator= (const rich_location &) = delete;
+
   /* Accessors.  */
   location_t get_loc () const { return get_loc (0); }
   location_t get_loc (unsigned int idx) const;