Re: [PATCH] libcpp: Improve encapsulation of label_text

[Jonathan Wakely <jwakely@redhat.com>]

On Thu, 14 Jul 2022 at 22:31, David Malcolm wrote:
>
> On Thu, 2022-07-14 at 22:10 +0100, Jonathan Wakely wrote:
>
> Thanks for the patch.
>
> > I'm not sure if label_text::get () is the best name for the new
> > accessor. Other options include buffer () and c_str () but I don't
> > see a
> > compelling reason to prefer either of those.
>
> label_text::get should return a const char *, rather than a char *.

OK. That requires a tweak to pod_label_text, as it wants to store the
result of get() as a char*.

> I don't think anything uses label_text::is_owner; do we need that?

The pod_label_text constructor that transfers ownership from a
label_text uses it.

An alternative to adding the is_owner accessor is to make
pod_label_text a friend of label_text. Alternatively, move
pod_label_text into libcpp and make it label_text know how to convert
itself to a pod_label_text (setting the bool correctly). That might be
overkill given that pod_label_text is used exactly once in one place,
but then arguably so is the is_owner() accessor that's needed exactly
once.


>
> >
> > As a follow-up patch we could change label_text::m_buffer (and
> > pod_label_text::m_buffer) to be const char*, since those labels are
> > never modified once a label_text takes ownership of it. That would
> > make it clear to callers that they are not supposed to modify the
> > contents, and would remove the const_cast currently used by
> > label_text::borrow (), which is a code smell (returning a non-const
> > char* makes it look like it's OK to write into it, which is
> > definitely
> > not true for a borrowed string that was originally a string literal).
> > That would require using const_cast when passing the buffer to free
> > for
> > deallocation, but that's fine, and avoids the impression that the
> > object
> > holds a modifiable string buffer.
>
> I'm not sure I agree with your reasoning about the proposed follow-up,

You mean you don't want the member to be a const char*? Works for me.
My goal is to stop users of label_text modifying it, and now that they
can only get to it via the get() accessor, that's enforced by making
the accessor return const. Changing the member isn't necessary.

> but the patch below is OK for trunk, with the above nits fixed.

Thanks, pushed to trunk.

Here's the incremental diff between the reviewed patch and what I pushed:

diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-locus.cc
index bcaa52ec779..9d430b5189c 100644
--- a/gcc/diagnostic-show-locus.cc
+++ b/gcc/diagnostic-show-locus.cc
@@ -1877,7 +1877,8 @@ struct pod_label_text
  {}

  pod_label_text (label_text &&other)
-  : m_buffer (other.get ()), m_caller_owned (other.is_owner ())
+  : m_buffer (const_cast<char*> (other.get ())),
+    m_caller_owned (other.is_owner ())
  {
    other.release ();
  }
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 193fec0a45f..9bdd5b9d30c 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1888,7 +1888,7 @@ public:
    m_owned = false;
  }

-  char *get () const
+  const char *get () const
  {
    return m_buffer;
  }