* [PATCH] libcpp: Improve encapsulation of label_text
@ 2022-07-14 21:10 Jonathan Wakely
2022-07-14 21:30 ` David Malcolm
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Wakely @ 2022-07-14 21:10 UTC (permalink / raw)
To: gcc-patches
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.
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.
Tested x86_64-linux, OK for trunk?
-- >8 --
This adjusts the API of label_text so that the data members are private
and cannot be modified by callers. Add accessors for them instead.
Also rename moved_from () to the more idiomatic release (). Also remove
the unused take_or_copy () member function which has confusing ownership
semantics.
gcc/analyzer/ChangeLog:
* call-info.cc (call_info::print): Adjust to new label_text API.
* checker-path.cc (checker_event::dump): Likewise.
(region_creation_event::get_desc): Likewise.
(state_change_event::get_desc): Likewise.
(superedge_event::should_filter_p): Likewise.
(start_cfg_edge_event::get_desc): Likewise.
(call_event::get_desc): Likewise.
(return_event::get_desc): Likewise.
(warning_event::get_desc): Likewise.
(checker_path::dump): Likewise.
(checker_path::debug): Likewise.
* diagnostic-manager.cc (diagnostic_manager::prune_for_sm_diagnostic):
Likewise.
(diagnostic_manager::prune_interproc_events): Likewise.
* engine.cc (feasibility_state::maybe_update_for_edge):
Likewise.
* program-state.cc (sm_state_map::to_json): Likewise.
* region-model-impl-calls.cc (region_model::impl_call_analyzer_describe): Likewise.
(region_model::impl_call_analyzer_dump_capacity): Likewise.
* region.cc (region::to_json): Likewise.
* sm-malloc.cc (inform_nonnull_attribute): Likewise.
* store.cc (binding_map::to_json): Likewise.
(store::to_json): Likewise.
* supergraph.cc (superedge::dump): Likewise.
* svalue.cc (svalue::to_json): Likewise.
gcc/c-family/ChangeLog:
* c-format.cc (class range_label_for_format_type_mismatch):
Adjust to new label_text API.
gcc/ChangeLog:
* diagnostic-format-json.cc (json_from_location_range): Adjust
to new label_text API.
* diagnostic-format-sarif.cc (sarif_builder::make_location_object):
Likewise.
* diagnostic-show-locus.cc (struct pod_label_text): Likewise.
(layout::print_any_labels): Likewise.
* tree-diagnostic-path.cc (class path_label): Likewise.
(struct event_range): Likewise.
(default_tree_diagnostic_path_printer): Likewise.
(default_tree_make_json_for_path): Likewise.
libcpp/ChangeLog:
* include/line-map.h (label_text::take_or_copy): Remove.
(label_text::moved_from): Rename to release.
(label_text::m_buffer, label_text::m_owned): Make private.
(label_text::get, label_text::is_owned): New accessors.
---
gcc/analyzer/call-info.cc | 2 +-
gcc/analyzer/checker-path.cc | 46 ++++++++++++-------------
gcc/analyzer/diagnostic-manager.cc | 20 +++++------
gcc/analyzer/engine.cc | 2 +-
gcc/analyzer/program-state.cc | 2 +-
gcc/analyzer/region-model-impl-calls.cc | 4 +--
gcc/analyzer/region.cc | 2 +-
gcc/analyzer/sm-malloc.cc | 10 +++---
gcc/analyzer/store.cc | 6 ++--
gcc/analyzer/supergraph.cc | 4 +--
gcc/analyzer/svalue.cc | 2 +-
gcc/c-family/c-format.cc | 4 +--
gcc/diagnostic-format-json.cc | 4 +--
gcc/diagnostic-format-sarif.cc | 2 +-
gcc/diagnostic-show-locus.cc | 6 ++--
gcc/tree-diagnostic-path.cc | 16 ++++-----
libcpp/include/line-map.h | 27 ++++++++-------
17 files changed, 80 insertions(+), 79 deletions(-)
diff --git a/gcc/analyzer/call-info.cc b/gcc/analyzer/call-info.cc
index e1142d743a3..efc070b8bed 100644
--- a/gcc/analyzer/call-info.cc
+++ b/gcc/analyzer/call-info.cc
@@ -75,7 +75,7 @@ void
call_info::print (pretty_printer *pp) const
{
label_text desc (get_desc (pp_show_color (pp)));
- pp_string (pp, desc.m_buffer);
+ pp_string (pp, desc.get ());
}
/* Implementation of custom_edge_info::add_events_to_path vfunc for
diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc
index 211cf3e0333..273f40d3a57 100644
--- a/gcc/analyzer/checker-path.cc
+++ b/gcc/analyzer/checker-path.cc
@@ -195,7 +195,7 @@ checker_event::dump (pretty_printer *pp) const
{
label_text event_desc (get_desc (false));
pp_printf (pp, "\"%s\" (depth %i",
- event_desc.m_buffer, m_effective_depth);
+ event_desc.get (), m_effective_depth);
if (m_effective_depth != m_original_depth)
pp_printf (pp, " corrected from %i",
@@ -307,7 +307,7 @@ region_creation_event::get_desc (bool can_colorize) const
label_text custom_desc
= m_pending_diagnostic->describe_region_creation_event
(evdesc::region_creation (can_colorize, m_reg));
- if (custom_desc.m_buffer)
+ if (custom_desc.get ())
return custom_desc;
}
@@ -390,7 +390,7 @@ state_change_event::get_desc (bool can_colorize) const
= m_pending_diagnostic->describe_state_change
(evdesc::state_change (can_colorize, var, origin,
m_from, m_to, m_emission_id, *this));
- if (custom_desc.m_buffer)
+ if (custom_desc.get ())
{
if (flag_analyzer_verbose_state_changes)
{
@@ -404,7 +404,7 @@ state_change_event::get_desc (bool can_colorize) const
return make_label_text
(can_colorize,
"%s (state of %qE: %qs -> %qs, origin: %qE, meaning: %s)",
- custom_desc.m_buffer,
+ custom_desc.get (),
var,
m_from->get_name (),
m_to->get_name (),
@@ -414,7 +414,7 @@ state_change_event::get_desc (bool can_colorize) const
return make_label_text
(can_colorize,
"%s (state of %qE: %qs -> %qs, NULL origin, meaning: %s)",
- custom_desc.m_buffer,
+ custom_desc.get (),
var,
m_from->get_name (),
m_to->get_name (),
@@ -435,16 +435,16 @@ state_change_event::get_desc (bool can_colorize) const
return make_label_text
(can_colorize,
"state of %qs: %qs -> %qs (origin: %qs)",
- sval_desc.m_buffer,
+ sval_desc.get (),
m_from->get_name (),
m_to->get_name (),
- origin_desc.m_buffer);
+ origin_desc.get ());
}
else
return make_label_text
(can_colorize,
"state of %qs: %qs -> %qs (NULL origin)",
- sval_desc.m_buffer,
+ sval_desc.get (),
m_from->get_name (),
m_to->get_name ());
}
@@ -509,8 +509,8 @@ superedge_event::should_filter_p (int verbosity) const
/* Filter events with empty descriptions. This ought to filter
FALLTHRU, but retain true/false/switch edges. */
label_text desc = get_desc (false);
- gcc_assert (desc.m_buffer);
- if (desc.m_buffer[0] == '\0')
+ gcc_assert (desc.get ());
+ if (desc.get ()[0] == '\0')
return true;
}
}
@@ -597,28 +597,28 @@ start_cfg_edge_event::get_desc (bool can_colorize) const
label_text edge_desc (m_sedge->get_description (user_facing));
if (user_facing)
{
- if (edge_desc.m_buffer && strlen (edge_desc.m_buffer) > 0)
+ if (edge_desc.get () && strlen (edge_desc.get ()) > 0)
{
label_text cond_desc = maybe_describe_condition (can_colorize);
label_text result;
- if (cond_desc.m_buffer)
+ if (cond_desc.get ())
return make_label_text (can_colorize,
"following %qs branch (%s)...",
- edge_desc.m_buffer, cond_desc.m_buffer);
+ edge_desc.get (), cond_desc.get ());
else
return make_label_text (can_colorize,
"following %qs branch...",
- edge_desc.m_buffer);
+ edge_desc.get ());
}
else
return label_text::borrow ("");
}
else
{
- if (strlen (edge_desc.m_buffer) > 0)
+ if (strlen (edge_desc.get ()) > 0)
return make_label_text (can_colorize,
"taking %qs edge SN:%i -> SN:%i",
- edge_desc.m_buffer,
+ edge_desc.get (),
m_sedge->m_src->m_index,
m_sedge->m_dest->m_index);
else
@@ -798,7 +798,7 @@ call_event::get_desc (bool can_colorize) const
m_dest_snode->m_fun->decl,
var,
m_critical_state));
- if (custom_desc.m_buffer)
+ if (custom_desc.get ())
return custom_desc;
}
@@ -878,7 +878,7 @@ return_event::get_desc (bool can_colorize) const
m_dest_snode->m_fun->decl,
m_src_snode->m_fun->decl,
m_critical_state));
- if (custom_desc.m_buffer)
+ if (custom_desc.get ())
return custom_desc;
}
return make_label_text (can_colorize,
@@ -1105,19 +1105,19 @@ warning_event::get_desc (bool can_colorize) const
label_text ev_desc
= m_pending_diagnostic->describe_final_event
(evdesc::final_event (can_colorize, var, m_state));
- if (ev_desc.m_buffer)
+ if (ev_desc.get ())
{
if (m_sm && flag_analyzer_verbose_state_changes)
{
if (var)
return make_label_text (can_colorize,
"%s (%qE is in state %qs)",
- ev_desc.m_buffer,
+ ev_desc.get (),
var, m_state->get_name ());
else
return make_label_text (can_colorize,
"%s (in global state %qs)",
- ev_desc.m_buffer,
+ ev_desc.get (),
m_state->get_name ());
}
else
@@ -1163,7 +1163,7 @@ checker_path::dump (pretty_printer *pp) const
if (i > 0)
pp_string (pp, ", ");
label_text event_desc (e->get_desc (false));
- pp_printf (pp, "\"%s\"", event_desc.m_buffer);
+ pp_printf (pp, "\"%s\"", event_desc.get ());
}
pp_character (pp, ']');
}
@@ -1203,7 +1203,7 @@ checker_path::debug () const
"[%i]: %s \"%s\"\n",
i,
event_kind_to_string (m_events[i]->m_kind),
- event_desc.m_buffer);
+ event_desc.get ());
}
}
diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc
index 083f66bd739..fded8281e57 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -2297,7 +2297,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
label_text sval_desc = sval->get_desc ();
log ("considering event %i (%s), with sval: %qs, state: %qs",
idx, event_kind_to_string (base_event->m_kind),
- sval_desc.m_buffer, state->get_name ());
+ sval_desc.get (), state->get_name ());
}
else
log ("considering event %i (%s), with global state: %qs",
@@ -2363,8 +2363,8 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
= state_change->m_origin->get_desc ();
log ("event %i:"
" switching var of interest from %qs to %qs",
- idx, sval_desc.m_buffer,
- origin_sval_desc.m_buffer);
+ idx, sval_desc.get (),
+ origin_sval_desc.get ());
}
sval = state_change->m_origin;
}
@@ -2386,12 +2386,12 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
label_text sval_desc = sval->get_desc ();
log ("filtering event %i:"
" state change to %qs unrelated to %qs",
- idx, change_sval_desc.m_buffer,
- sval_desc.m_buffer);
+ idx, change_sval_desc.get (),
+ sval_desc.get ());
}
else
log ("filtering event %i: state change to %qs",
- idx, change_sval_desc.m_buffer);
+ idx, change_sval_desc.get ());
}
else
log ("filtering event %i: global state change", idx);
@@ -2460,7 +2460,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
log ("event %i:"
" recording critical state for %qs at call"
" from %qE in callee to %qE in caller",
- idx, sval_desc.m_buffer, callee_var, caller_var);
+ idx, sval_desc.get (), callee_var, caller_var);
}
if (expr.param_p ())
event->record_critical_state (caller_var, state);
@@ -2503,7 +2503,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
log ("event %i:"
" recording critical state for %qs at return"
" from %qE in caller to %qE in callee",
- idx, sval_desc.m_buffer, callee_var, callee_var);
+ idx, sval_desc.get (), callee_var, callee_var);
}
if (expr.return_value_p ())
event->record_critical_state (callee_var, state);
@@ -2586,7 +2586,7 @@ diagnostic_manager::prune_interproc_events (checker_path *path) const
(path->get_checker_event (idx)->get_desc (false));
log ("filtering events %i-%i:"
" irrelevant call/entry/return: %s",
- idx, idx + 2, desc.m_buffer);
+ idx, idx + 2, desc.get ());
}
path->delete_event (idx + 2);
path->delete_event (idx + 1);
@@ -2608,7 +2608,7 @@ diagnostic_manager::prune_interproc_events (checker_path *path) const
(path->get_checker_event (idx)->get_desc (false));
log ("filtering events %i-%i:"
" irrelevant call/return: %s",
- idx, idx + 1, desc.m_buffer);
+ idx, idx + 1, desc.get ());
}
path->delete_event (idx + 1);
path->delete_event (idx);
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 888123f2b95..9ffcc410839 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -4590,7 +4590,7 @@ feasibility_state::maybe_update_for_edge (logger *logger,
logger->log (" sedge: SN:%i -> SN:%i %s",
sedge->m_src->m_index,
sedge->m_dest->m_index,
- desc.m_buffer);
+ desc.get ());
}
const gimple *last_stmt = src_point.get_supernode ()->get_last_stmt ();
diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 90a56e3fba4..f0f40465aad 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -300,7 +300,7 @@ sm_state_map::to_json () const
entry_t e = (*iter).second;
label_text sval_desc = sval->get_desc ();
- map_obj->set (sval_desc.m_buffer, e.m_state->to_json ());
+ map_obj->set (sval_desc.get (), e.m_state->to_json ());
/* This doesn't yet JSONify e.m_origin. */
}
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 55d6fa7f76b..8c38e9206fa 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -255,7 +255,7 @@ region_model::impl_call_analyzer_describe (const gcall *call,
const svalue *sval = get_rvalue (t_val, ctxt);
bool simple = zerop (t_verbosity);
label_text desc = sval->get_desc (simple);
- warning_at (call->location, 0, "svalue: %qs", desc.m_buffer);
+ warning_at (call->location, 0, "svalue: %qs", desc.get ());
}
/* Handle a call to "__analyzer_dump_capacity".
@@ -274,7 +274,7 @@ region_model::impl_call_analyzer_dump_capacity (const gcall *call,
const region *base_reg = reg->get_base_region ();
const svalue *capacity = get_capacity (base_reg);
label_text desc = capacity->get_desc (true);
- warning_at (call->location, 0, "capacity: %qs", desc.m_buffer);
+ warning_at (call->location, 0, "capacity: %qs", desc.get ());
}
/* Compare D1 and D2 using their names, and then IDs to order them. */
diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc
index 5b00e6a5f46..a8d1ae92deb 100644
--- a/gcc/analyzer/region.cc
+++ b/gcc/analyzer/region.cc
@@ -589,7 +589,7 @@ json::value *
region::to_json () const
{
label_text desc = get_desc (true);
- json::value *reg_js = new json::string (desc.m_buffer);
+ json::value *reg_js = new json::string (desc.get ());
return reg_js;
}
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 553fcd80085..608aceb1abe 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -1007,7 +1007,7 @@ inform_nonnull_attribute (tree fndecl, int arg_idx)
label_text arg_desc = describe_argument_index (fndecl, arg_idx);
inform (DECL_SOURCE_LOCATION (fndecl),
"argument %s of %qD must be non-null",
- arg_desc.m_buffer, fndecl);
+ arg_desc.get (), fndecl);
/* Ideally we would use the location of the parm and underline the
attribute also - but we don't have the location_t values at this point
in the middle-end.
@@ -1065,12 +1065,12 @@ public:
if (m_origin_of_unchecked_event.known_p ())
result = ev.formatted_print ("argument %s (%qE) from %@ could be NULL"
" where non-null expected",
- arg_desc.m_buffer, ev.m_expr,
+ arg_desc.get (), ev.m_expr,
&m_origin_of_unchecked_event);
else
result = ev.formatted_print ("argument %s (%qE) could be NULL"
" where non-null expected",
- arg_desc.m_buffer, ev.m_expr);
+ arg_desc.get (), ev.m_expr);
return result;
}
@@ -1173,11 +1173,11 @@ public:
label_text result;
if (zerop (ev.m_expr))
result = ev.formatted_print ("argument %s NULL where non-null expected",
- arg_desc.m_buffer);
+ arg_desc.get ());
else
result = ev.formatted_print ("argument %s (%qE) NULL"
" where non-null expected",
- arg_desc.m_buffer, ev.m_expr);
+ arg_desc.get (), ev.m_expr);
return result;
}
diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index d558d477115..06151d8c041 100644
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -675,7 +675,7 @@ binding_map::to_json () const
{
const svalue *value = *const_cast <map_t &> (m_map).get (key);
label_text key_desc = key->get_desc ();
- map_obj->set (key_desc.m_buffer, value->to_json ());
+ map_obj->set (key_desc.get (), value->to_json ());
}
return map_obj;
@@ -2402,11 +2402,11 @@ store::to_json () const
binding_cluster *cluster
= *const_cast<cluster_map_t &> (m_cluster_map).get (base_reg);
label_text base_reg_desc = base_reg->get_desc ();
- clusters_in_parent_reg_obj->set (base_reg_desc.m_buffer,
+ clusters_in_parent_reg_obj->set (base_reg_desc.get (),
cluster->to_json ());
}
label_text parent_reg_desc = parent_reg->get_desc ();
- store_obj->set (parent_reg_desc.m_buffer, clusters_in_parent_reg_obj);
+ store_obj->set (parent_reg_desc.get (), clusters_in_parent_reg_obj);
}
store_obj->set ("called_unknown_fn", new json::literal (m_called_unknown_fn));
diff --git a/gcc/analyzer/supergraph.cc b/gcc/analyzer/supergraph.cc
index 52b4852404d..01e30f7f6d0 100644
--- a/gcc/analyzer/supergraph.cc
+++ b/gcc/analyzer/supergraph.cc
@@ -855,10 +855,10 @@ superedge::dump (pretty_printer *pp) const
{
pp_printf (pp, "edge: SN: %i -> SN: %i", m_src->m_index, m_dest->m_index);
label_text desc (get_description (false));
- if (strlen (desc.m_buffer) > 0)
+ if (strlen (desc.get ()) > 0)
{
pp_space (pp);
- pp_string (pp, desc.m_buffer);
+ pp_string (pp, desc.get ());
}
}
diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc
index 78a6eeff05f..f5a5f1c9697 100644
--- a/gcc/analyzer/svalue.cc
+++ b/gcc/analyzer/svalue.cc
@@ -96,7 +96,7 @@ json::value *
svalue::to_json () const
{
label_text desc = get_desc (true);
- json::value *sval_js = new json::string (desc.m_buffer);
+ json::value *sval_js = new json::string (desc.get ());
return sval_js;
}
diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
index 2faed0c1607..68b94da40cc 100644
--- a/gcc/c-family/c-format.cc
+++ b/gcc/c-family/c-format.cc
@@ -4617,14 +4617,14 @@ class range_label_for_format_type_mismatch
label_text get_text (unsigned range_idx) const final override
{
label_text text = range_label_for_type_mismatch::get_text (range_idx);
- if (text.m_buffer == NULL)
+ if (text.get () == NULL)
return text;
indirection_suffix suffix (m_pointer_count);
char *p = (char *) alloca (suffix.get_buffer_size ());
suffix.fill_buffer (p);
- char *result = concat (text.m_buffer, p, NULL);
+ char *result = concat (text.get (), p, NULL);
return label_text::take (result);
}
diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc
index 872c67e53ed..baadc4b27c9 100644
--- a/gcc/diagnostic-format-json.cc
+++ b/gcc/diagnostic-format-json.cc
@@ -102,8 +102,8 @@ json_from_location_range (diagnostic_context *context,
if (loc_range->m_label)
{
label_text text (loc_range->m_label->get_text (range_idx));
- if (text.m_buffer)
- result->set ("label", new json::string (text.m_buffer));
+ if (text.get ())
+ result->set ("label", new json::string (text.get ()));
}
return result;
diff --git a/gcc/diagnostic-format-sarif.cc b/gcc/diagnostic-format-sarif.cc
index 1e4ebc8ad38..fc28d160c38 100644
--- a/gcc/diagnostic-format-sarif.cc
+++ b/gcc/diagnostic-format-sarif.cc
@@ -582,7 +582,7 @@ sarif_builder::make_location_object (const diagnostic_event &event)
/* "message" property (SARIF v2.1.0 section 3.28.5). */
label_text ev_desc = event.get_desc (false);
- json::object *message_obj = make_message_object (ev_desc.m_buffer);
+ json::object *message_obj = make_message_object (ev_desc.get ());
location_obj->set ("message", message_obj);
return location_obj;
diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-locus.cc
index 08dab20e6cd..bcaa52ec779 100644
--- a/gcc/diagnostic-show-locus.cc
+++ b/gcc/diagnostic-show-locus.cc
@@ -1877,9 +1877,9 @@ struct pod_label_text
{}
pod_label_text (label_text &&other)
- : m_buffer (other.m_buffer), m_caller_owned (other.m_owned)
+ : m_buffer (other.get ()), m_caller_owned (other.is_owner ())
{
- other.moved_from ();
+ other.release ();
}
void maybe_free ()
@@ -1963,7 +1963,7 @@ layout::print_any_labels (linenum_type row)
/* Allow for labels that return NULL from their get_text
implementation (so e.g. such labels can control their own
visibility). */
- if (text.m_buffer == NULL)
+ if (text.get () == NULL)
continue;
labels.safe_push (line_label (m_policy, i, disp_col, std::move (text)));
diff --git a/gcc/tree-diagnostic-path.cc b/gcc/tree-diagnostic-path.cc
index 2f297faed34..6612b617471 100644
--- a/gcc/tree-diagnostic-path.cc
+++ b/gcc/tree-diagnostic-path.cc
@@ -61,11 +61,11 @@ class path_label : public range_label
is special-cased for diagnostic paths. */
bool colorize = pp_show_color (global_dc->printer);
label_text event_text (event.get_desc (colorize));
- gcc_assert (event_text.m_buffer);
+ gcc_assert (event_text.get ());
pretty_printer pp;
pp_show_color (&pp) = pp_show_color (global_dc->printer);
diagnostic_event_id_t event_id (event_idx);
- pp_printf (&pp, "%@ %s", &event_id, event_text.m_buffer);
+ pp_printf (&pp, "%@ %s", &event_id, event_text.get ());
label_text result = label_text::take (xstrdup (pp_formatted_text (&pp)));
return result;
}
@@ -173,7 +173,7 @@ struct event_range
diagnostic_event_id_t event_id (i);
label_text event_text (iter_event.get_desc (true));
pretty_printer *pp = dc->printer;
- pp_printf (pp, " %@: %s", &event_id, event_text.m_buffer);
+ pp_printf (pp, " %@: %s", &event_id, event_text.get ());
pp_newline (pp);
}
return;
@@ -459,7 +459,7 @@ default_tree_diagnostic_path_printer (diagnostic_context *context,
{
const diagnostic_event &event = path->get_event (i);
label_text event_text (event.get_desc (false));
- gcc_assert (event_text.m_buffer);
+ gcc_assert (event_text.get ());
diagnostic_event_id_t event_id (i);
if (context->show_path_depths)
{
@@ -471,17 +471,17 @@ default_tree_diagnostic_path_printer (diagnostic_context *context,
if (fndecl)
inform (event.get_location (),
"%@ %s (fndecl %qD, depth %i)",
- &event_id, event_text.m_buffer,
+ &event_id, event_text.get (),
fndecl, stack_depth);
else
inform (event.get_location (),
"%@ %s (depth %i)",
- &event_id, event_text.m_buffer,
+ &event_id, event_text.get (),
stack_depth);
}
else
inform (event.get_location (),
- "%@ %s", &event_id, event_text.m_buffer);
+ "%@ %s", &event_id, event_text.get ());
}
}
break;
@@ -519,7 +519,7 @@ default_tree_make_json_for_path (diagnostic_context *context,
json_from_expanded_location (context,
event.get_location ()));
label_text event_text (event.get_desc (false));
- event_obj->set ("description", new json::string (event_text.m_buffer));
+ event_obj->set ("description", new json::string (event_text.get ()));
if (tree fndecl = event.get_fndecl ())
{
const char *function
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index c434a246b13..193fec0a45f 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1851,7 +1851,7 @@ public:
label_text (label_text &&other)
: m_buffer (other.m_buffer), m_owned (other.m_owned)
{
- other.moved_from ();
+ other.release ();
}
/* Move assignment. */
@@ -1861,7 +1861,7 @@ public:
free (m_buffer);
m_buffer = other.m_buffer;
m_owned = other.m_owned;
- other.moved_from ();
+ other.release ();
return *this;
}
@@ -1882,25 +1882,26 @@ public:
return label_text (buffer, true);
}
- /* Take ownership of the buffer, copying if necessary. */
- char *take_or_copy ()
- {
- if (m_owned)
- return m_buffer;
- else
- return xstrdup (m_buffer);
- }
-
- void moved_from ()
+ void release ()
{
m_buffer = NULL;
m_owned = false;
}
+ char *get () const
+ {
+ return m_buffer;
+ }
+
+ bool is_owner () const
+ {
+ return m_owned;
+ }
+
+private:
char *m_buffer;
bool m_owned;
-private:
label_text (char *buffer, bool owned)
: m_buffer (buffer), m_owned (owned)
{}
--
2.36.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libcpp: Improve encapsulation of label_text
2022-07-14 21:10 [PATCH] libcpp: Improve encapsulation of label_text Jonathan Wakely
@ 2022-07-14 21:30 ` David Malcolm
2022-07-15 8:44 ` Jonathan Wakely
0 siblings, 1 reply; 3+ messages in thread
From: David Malcolm @ 2022-07-14 21:30 UTC (permalink / raw)
To: Jonathan Wakely, gcc-patches
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 *.
I don't think anything uses label_text::is_owner; do we need that?
>
> 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,
but the patch below is OK for trunk, with the above nits fixed.
Thanks
Dave
>
> Tested x86_64-linux, OK for trunk?
>
>
> -- >8 --
>
> This adjusts the API of label_text so that the data members are
> private
> and cannot be modified by callers. Add accessors for them instead.
> Also rename moved_from () to the more idiomatic release (). Also
> remove
> the unused take_or_copy () member function which has confusing
> ownership
> semantics.
>
> gcc/analyzer/ChangeLog:
>
> * call-info.cc (call_info::print): Adjust to new label_text
> API.
> * checker-path.cc (checker_event::dump): Likewise.
> (region_creation_event::get_desc): Likewise.
> (state_change_event::get_desc): Likewise.
> (superedge_event::should_filter_p): Likewise.
> (start_cfg_edge_event::get_desc): Likewise.
> (call_event::get_desc): Likewise.
> (return_event::get_desc): Likewise.
> (warning_event::get_desc): Likewise.
> (checker_path::dump): Likewise.
> (checker_path::debug): Likewise.
> * diagnostic-manager.cc
> (diagnostic_manager::prune_for_sm_diagnostic):
> Likewise.
> (diagnostic_manager::prune_interproc_events): Likewise.
> * engine.cc (feasibility_state::maybe_update_for_edge):
> Likewise.
> * program-state.cc (sm_state_map::to_json): Likewise.
> * region-model-impl-calls.cc
> (region_model::impl_call_analyzer_describe): Likewise.
> (region_model::impl_call_analyzer_dump_capacity): Likewise.
> * region.cc (region::to_json): Likewise.
> * sm-malloc.cc (inform_nonnull_attribute): Likewise.
> * store.cc (binding_map::to_json): Likewise.
> (store::to_json): Likewise.
> * supergraph.cc (superedge::dump): Likewise.
> * svalue.cc (svalue::to_json): Likewise.
>
> gcc/c-family/ChangeLog:
>
> * c-format.cc (class range_label_for_format_type_mismatch):
> Adjust to new label_text API.
>
> gcc/ChangeLog:
>
> * diagnostic-format-json.cc (json_from_location_range):
> Adjust
> to new label_text API.
> * diagnostic-format-sarif.cc
> (sarif_builder::make_location_object):
> Likewise.
> * diagnostic-show-locus.cc (struct pod_label_text): Likewise.
> (layout::print_any_labels): Likewise.
> * tree-diagnostic-path.cc (class path_label): Likewise.
> (struct event_range): Likewise.
> (default_tree_diagnostic_path_printer): Likewise.
> (default_tree_make_json_for_path): Likewise.
>
> libcpp/ChangeLog:
>
> * include/line-map.h (label_text::take_or_copy): Remove.
> (label_text::moved_from): Rename to release.
> (label_text::m_buffer, label_text::m_owned): Make private.
> (label_text::get, label_text::is_owned): New accessors.
> ---
> gcc/analyzer/call-info.cc | 2 +-
> gcc/analyzer/checker-path.cc | 46 ++++++++++++-----------
> --
> gcc/analyzer/diagnostic-manager.cc | 20 +++++------
> gcc/analyzer/engine.cc | 2 +-
> gcc/analyzer/program-state.cc | 2 +-
> gcc/analyzer/region-model-impl-calls.cc | 4 +--
> gcc/analyzer/region.cc | 2 +-
> gcc/analyzer/sm-malloc.cc | 10 +++---
> gcc/analyzer/store.cc | 6 ++--
> gcc/analyzer/supergraph.cc | 4 +--
> gcc/analyzer/svalue.cc | 2 +-
> gcc/c-family/c-format.cc | 4 +--
> gcc/diagnostic-format-json.cc | 4 +--
> gcc/diagnostic-format-sarif.cc | 2 +-
> gcc/diagnostic-show-locus.cc | 6 ++--
> gcc/tree-diagnostic-path.cc | 16 ++++-----
> libcpp/include/line-map.h | 27 ++++++++-------
> 17 files changed, 80 insertions(+), 79 deletions(-)
>
> diff --git a/gcc/analyzer/call-info.cc b/gcc/analyzer/call-info.cc
> index e1142d743a3..efc070b8bed 100644
> --- a/gcc/analyzer/call-info.cc
> +++ b/gcc/analyzer/call-info.cc
> @@ -75,7 +75,7 @@ void
> call_info::print (pretty_printer *pp) const
> {
> label_text desc (get_desc (pp_show_color (pp)));
> - pp_string (pp, desc.m_buffer);
> + pp_string (pp, desc.get ());
> }
>
> /* Implementation of custom_edge_info::add_events_to_path vfunc for
> diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-
> path.cc
> index 211cf3e0333..273f40d3a57 100644
> --- a/gcc/analyzer/checker-path.cc
> +++ b/gcc/analyzer/checker-path.cc
> @@ -195,7 +195,7 @@ checker_event::dump (pretty_printer *pp) const
> {
> label_text event_desc (get_desc (false));
> pp_printf (pp, "\"%s\" (depth %i",
> - event_desc.m_buffer, m_effective_depth);
> + event_desc.get (), m_effective_depth);
>
> if (m_effective_depth != m_original_depth)
> pp_printf (pp, " corrected from %i",
> @@ -307,7 +307,7 @@ region_creation_event::get_desc (bool
> can_colorize) const
> label_text custom_desc
> = m_pending_diagnostic->describe_region_creation_event
> (evdesc::region_creation (can_colorize, m_reg));
> - if (custom_desc.m_buffer)
> + if (custom_desc.get ())
> return custom_desc;
> }
>
> @@ -390,7 +390,7 @@ state_change_event::get_desc (bool can_colorize)
> const
> = m_pending_diagnostic->describe_state_change
> (evdesc::state_change (can_colorize, var, origin,
> m_from, m_to, m_emission_id,
> *this));
> - if (custom_desc.m_buffer)
> + if (custom_desc.get ())
> {
> if (flag_analyzer_verbose_state_changes)
> {
> @@ -404,7 +404,7 @@ state_change_event::get_desc (bool can_colorize)
> const
> return make_label_text
> (can_colorize,
> "%s (state of %qE: %qs -> %qs, origin: %qE,
> meaning: %s)",
> - custom_desc.m_buffer,
> + custom_desc.get (),
> var,
> m_from->get_name (),
> m_to->get_name (),
> @@ -414,7 +414,7 @@ state_change_event::get_desc (bool can_colorize)
> const
> return make_label_text
> (can_colorize,
> "%s (state of %qE: %qs -> %qs, NULL origin,
> meaning: %s)",
> - custom_desc.m_buffer,
> + custom_desc.get (),
> var,
> m_from->get_name (),
> m_to->get_name (),
> @@ -435,16 +435,16 @@ state_change_event::get_desc (bool
> can_colorize) const
> return make_label_text
> (can_colorize,
> "state of %qs: %qs -> %qs (origin: %qs)",
> - sval_desc.m_buffer,
> + sval_desc.get (),
> m_from->get_name (),
> m_to->get_name (),
> - origin_desc.m_buffer);
> + origin_desc.get ());
> }
> else
> return make_label_text
> (can_colorize,
> "state of %qs: %qs -> %qs (NULL origin)",
> - sval_desc.m_buffer,
> + sval_desc.get (),
> m_from->get_name (),
> m_to->get_name ());
> }
> @@ -509,8 +509,8 @@ superedge_event::should_filter_p (int verbosity)
> const
> /* Filter events with empty descriptions. This ought to
> filter
> FALLTHRU, but retain true/false/switch edges. */
> label_text desc = get_desc (false);
> - gcc_assert (desc.m_buffer);
> - if (desc.m_buffer[0] == '\0')
> + gcc_assert (desc.get ());
> + if (desc.get ()[0] == '\0')
> return true;
> }
> }
> @@ -597,28 +597,28 @@ start_cfg_edge_event::get_desc (bool
> can_colorize) const
> label_text edge_desc (m_sedge->get_description (user_facing));
> if (user_facing)
> {
> - if (edge_desc.m_buffer && strlen (edge_desc.m_buffer) > 0)
> + if (edge_desc.get () && strlen (edge_desc.get ()) > 0)
> {
> label_text cond_desc = maybe_describe_condition
> (can_colorize);
> label_text result;
> - if (cond_desc.m_buffer)
> + if (cond_desc.get ())
> return make_label_text (can_colorize,
> "following %qs branch (%s)...",
> - edge_desc.m_buffer,
> cond_desc.m_buffer);
> + edge_desc.get (), cond_desc.get
> ());
> else
> return make_label_text (can_colorize,
> "following %qs branch...",
> - edge_desc.m_buffer);
> + edge_desc.get ());
> }
> else
> return label_text::borrow ("");
> }
> else
> {
> - if (strlen (edge_desc.m_buffer) > 0)
> + if (strlen (edge_desc.get ()) > 0)
> return make_label_text (can_colorize,
> "taking %qs edge SN:%i -> SN:%i",
> - edge_desc.m_buffer,
> + edge_desc.get (),
> m_sedge->m_src->m_index,
> m_sedge->m_dest->m_index);
> else
> @@ -798,7 +798,7 @@ call_event::get_desc (bool can_colorize) const
> m_dest_snode->m_fun->decl,
> var,
> m_critical_state));
> - if (custom_desc.m_buffer)
> + if (custom_desc.get ())
> return custom_desc;
> }
>
> @@ -878,7 +878,7 @@ return_event::get_desc (bool can_colorize) const
> m_dest_snode->m_fun->decl,
> m_src_snode->m_fun->decl,
> m_critical_state));
> - if (custom_desc.m_buffer)
> + if (custom_desc.get ())
> return custom_desc;
> }
> return make_label_text (can_colorize,
> @@ -1105,19 +1105,19 @@ warning_event::get_desc (bool can_colorize)
> const
> label_text ev_desc
> = m_pending_diagnostic->describe_final_event
> (evdesc::final_event (can_colorize, var, m_state));
> - if (ev_desc.m_buffer)
> + if (ev_desc.get ())
> {
> if (m_sm && flag_analyzer_verbose_state_changes)
> {
> if (var)
> return make_label_text (can_colorize,
> "%s (%qE is in state %qs)",
> - ev_desc.m_buffer,
> + ev_desc.get (),
> var, m_state->get_name ());
> else
> return make_label_text (can_colorize,
> "%s (in global state %qs)",
> - ev_desc.m_buffer,
> + ev_desc.get (),
> m_state->get_name ());
> }
> else
> @@ -1163,7 +1163,7 @@ checker_path::dump (pretty_printer *pp) const
> if (i > 0)
> pp_string (pp, ", ");
> label_text event_desc (e->get_desc (false));
> - pp_printf (pp, "\"%s\"", event_desc.m_buffer);
> + pp_printf (pp, "\"%s\"", event_desc.get ());
> }
> pp_character (pp, ']');
> }
> @@ -1203,7 +1203,7 @@ checker_path::debug () const
> "[%i]: %s \"%s\"\n",
> i,
> event_kind_to_string (m_events[i]->m_kind),
> - event_desc.m_buffer);
> + event_desc.get ());
> }
> }
>
> diff --git a/gcc/analyzer/diagnostic-manager.cc
> b/gcc/analyzer/diagnostic-manager.cc
> index 083f66bd739..fded8281e57 100644
> --- a/gcc/analyzer/diagnostic-manager.cc
> +++ b/gcc/analyzer/diagnostic-manager.cc
> @@ -2297,7 +2297,7 @@ diagnostic_manager::prune_for_sm_diagnostic
> (checker_path *path,
> label_text sval_desc = sval->get_desc ();
> log ("considering event %i (%s), with sval: %qs,
> state: %qs",
> idx, event_kind_to_string (base_event-
> >m_kind),
> - sval_desc.m_buffer, state->get_name ());
> + sval_desc.get (), state->get_name ());
> }
> else
> log ("considering event %i (%s), with global state:
> %qs",
> @@ -2363,8 +2363,8 @@ diagnostic_manager::prune_for_sm_diagnostic
> (checker_path *path,
> = state_change->m_origin->get_desc ();
> log ("event %i:"
> " switching var of interest from %qs to
> %qs",
> - idx, sval_desc.m_buffer,
> - origin_sval_desc.m_buffer);
> + idx, sval_desc.get (),
> + origin_sval_desc.get ());
> }
> sval = state_change->m_origin;
> }
> @@ -2386,12 +2386,12 @@ diagnostic_manager::prune_for_sm_diagnostic
> (checker_path *path,
> label_text sval_desc = sval->get_desc ();
> log ("filtering event %i:"
> " state change to %qs unrelated to
> %qs",
> - idx, change_sval_desc.m_buffer,
> - sval_desc.m_buffer);
> + idx, change_sval_desc.get (),
> + sval_desc.get ());
> }
> else
> log ("filtering event %i: state change to
> %qs",
> - idx, change_sval_desc.m_buffer);
> + idx, change_sval_desc.get ());
> }
> else
> log ("filtering event %i: global state change",
> idx);
> @@ -2460,7 +2460,7 @@ diagnostic_manager::prune_for_sm_diagnostic
> (checker_path *path,
> log ("event %i:"
> " recording critical state for %qs at call"
> " from %qE in callee to %qE in caller",
> - idx, sval_desc.m_buffer, callee_var,
> caller_var);
> + idx, sval_desc.get (), callee_var,
> caller_var);
> }
> if (expr.param_p ())
> event->record_critical_state (caller_var, state);
> @@ -2503,7 +2503,7 @@ diagnostic_manager::prune_for_sm_diagnostic
> (checker_path *path,
> log ("event %i:"
> " recording critical state for %qs at
> return"
> " from %qE in caller to %qE in callee",
> - idx, sval_desc.m_buffer, callee_var,
> callee_var);
> + idx, sval_desc.get (), callee_var,
> callee_var);
> }
> if (expr.return_value_p ())
> event->record_critical_state (callee_var,
> state);
> @@ -2586,7 +2586,7 @@ diagnostic_manager::prune_interproc_events
> (checker_path *path) const
> (path->get_checker_event (idx)->get_desc
> (false));
> log ("filtering events %i-%i:"
> " irrelevant call/entry/return: %s",
> - idx, idx + 2, desc.m_buffer);
> + idx, idx + 2, desc.get ());
> }
> path->delete_event (idx + 2);
> path->delete_event (idx + 1);
> @@ -2608,7 +2608,7 @@ diagnostic_manager::prune_interproc_events
> (checker_path *path) const
> (path->get_checker_event (idx)->get_desc
> (false));
> log ("filtering events %i-%i:"
> " irrelevant call/return: %s",
> - idx, idx + 1, desc.m_buffer);
> + idx, idx + 1, desc.get ());
> }
> path->delete_event (idx + 1);
> path->delete_event (idx);
> diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
> index 888123f2b95..9ffcc410839 100644
> --- a/gcc/analyzer/engine.cc
> +++ b/gcc/analyzer/engine.cc
> @@ -4590,7 +4590,7 @@ feasibility_state::maybe_update_for_edge
> (logger *logger,
> logger->log (" sedge: SN:%i -> SN:%i %s",
> sedge->m_src->m_index,
> sedge->m_dest->m_index,
> - desc.m_buffer);
> + desc.get ());
> }
>
> const gimple *last_stmt = src_point.get_supernode ()-
> >get_last_stmt ();
> diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-
> state.cc
> index 90a56e3fba4..f0f40465aad 100644
> --- a/gcc/analyzer/program-state.cc
> +++ b/gcc/analyzer/program-state.cc
> @@ -300,7 +300,7 @@ sm_state_map::to_json () const
> entry_t e = (*iter).second;
>
> label_text sval_desc = sval->get_desc ();
> - map_obj->set (sval_desc.m_buffer, e.m_state->to_json ());
> + map_obj->set (sval_desc.get (), e.m_state->to_json ());
>
> /* This doesn't yet JSONify e.m_origin. */
> }
> diff --git a/gcc/analyzer/region-model-impl-calls.cc
> b/gcc/analyzer/region-model-impl-calls.cc
> index 55d6fa7f76b..8c38e9206fa 100644
> --- a/gcc/analyzer/region-model-impl-calls.cc
> +++ b/gcc/analyzer/region-model-impl-calls.cc
> @@ -255,7 +255,7 @@ region_model::impl_call_analyzer_describe (const
> gcall *call,
> const svalue *sval = get_rvalue (t_val, ctxt);
> bool simple = zerop (t_verbosity);
> label_text desc = sval->get_desc (simple);
> - warning_at (call->location, 0, "svalue: %qs", desc.m_buffer);
> + warning_at (call->location, 0, "svalue: %qs", desc.get ());
> }
>
> /* Handle a call to "__analyzer_dump_capacity".
> @@ -274,7 +274,7 @@ region_model::impl_call_analyzer_dump_capacity
> (const gcall *call,
> const region *base_reg = reg->get_base_region ();
> const svalue *capacity = get_capacity (base_reg);
> label_text desc = capacity->get_desc (true);
> - warning_at (call->location, 0, "capacity: %qs", desc.m_buffer);
> + warning_at (call->location, 0, "capacity: %qs", desc.get ());
> }
>
> /* Compare D1 and D2 using their names, and then IDs to order them.
> */
> diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc
> index 5b00e6a5f46..a8d1ae92deb 100644
> --- a/gcc/analyzer/region.cc
> +++ b/gcc/analyzer/region.cc
> @@ -589,7 +589,7 @@ json::value *
> region::to_json () const
> {
> label_text desc = get_desc (true);
> - json::value *reg_js = new json::string (desc.m_buffer);
> + json::value *reg_js = new json::string (desc.get ());
> return reg_js;
> }
>
> diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
> index 553fcd80085..608aceb1abe 100644
> --- a/gcc/analyzer/sm-malloc.cc
> +++ b/gcc/analyzer/sm-malloc.cc
> @@ -1007,7 +1007,7 @@ inform_nonnull_attribute (tree fndecl, int
> arg_idx)
> label_text arg_desc = describe_argument_index (fndecl, arg_idx);
> inform (DECL_SOURCE_LOCATION (fndecl),
> "argument %s of %qD must be non-null",
> - arg_desc.m_buffer, fndecl);
> + arg_desc.get (), fndecl);
> /* Ideally we would use the location of the parm and underline the
> attribute also - but we don't have the location_t values at
> this point
> in the middle-end.
> @@ -1065,12 +1065,12 @@ public:
> if (m_origin_of_unchecked_event.known_p ())
> result = ev.formatted_print ("argument %s (%qE) from %@ could
> be NULL"
> " where non-null expected",
> - arg_desc.m_buffer, ev.m_expr,
> + arg_desc.get (), ev.m_expr,
> &m_origin_of_unchecked_event);
> else
> result = ev.formatted_print ("argument %s (%qE) could be NULL"
> " where non-null expected",
> - arg_desc.m_buffer, ev.m_expr);
> + arg_desc.get (), ev.m_expr);
> return result;
> }
>
> @@ -1173,11 +1173,11 @@ public:
> label_text result;
> if (zerop (ev.m_expr))
> result = ev.formatted_print ("argument %s NULL where non-null
> expected",
> - arg_desc.m_buffer);
> + arg_desc.get ());
> else
> result = ev.formatted_print ("argument %s (%qE) NULL"
> " where non-null expected",
> - arg_desc.m_buffer, ev.m_expr);
> + arg_desc.get (), ev.m_expr);
> return result;
> }
>
> diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
> index d558d477115..06151d8c041 100644
> --- a/gcc/analyzer/store.cc
> +++ b/gcc/analyzer/store.cc
> @@ -675,7 +675,7 @@ binding_map::to_json () const
> {
> const svalue *value = *const_cast <map_t &> (m_map).get (key);
> label_text key_desc = key->get_desc ();
> - map_obj->set (key_desc.m_buffer, value->to_json ());
> + map_obj->set (key_desc.get (), value->to_json ());
> }
>
> return map_obj;
> @@ -2402,11 +2402,11 @@ store::to_json () const
> binding_cluster *cluster
> = *const_cast<cluster_map_t &> (m_cluster_map).get
> (base_reg);
> label_text base_reg_desc = base_reg->get_desc ();
> - clusters_in_parent_reg_obj->set (base_reg_desc.m_buffer,
> + clusters_in_parent_reg_obj->set (base_reg_desc.get (),
> cluster->to_json ());
> }
> label_text parent_reg_desc = parent_reg->get_desc ();
> - store_obj->set (parent_reg_desc.m_buffer,
> clusters_in_parent_reg_obj);
> + store_obj->set (parent_reg_desc.get (),
> clusters_in_parent_reg_obj);
> }
>
> store_obj->set ("called_unknown_fn", new json::literal
> (m_called_unknown_fn));
> diff --git a/gcc/analyzer/supergraph.cc b/gcc/analyzer/supergraph.cc
> index 52b4852404d..01e30f7f6d0 100644
> --- a/gcc/analyzer/supergraph.cc
> +++ b/gcc/analyzer/supergraph.cc
> @@ -855,10 +855,10 @@ superedge::dump (pretty_printer *pp) const
> {
> pp_printf (pp, "edge: SN: %i -> SN: %i", m_src->m_index, m_dest-
> >m_index);
> label_text desc (get_description (false));
> - if (strlen (desc.m_buffer) > 0)
> + if (strlen (desc.get ()) > 0)
> {
> pp_space (pp);
> - pp_string (pp, desc.m_buffer);
> + pp_string (pp, desc.get ());
> }
> }
>
> diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc
> index 78a6eeff05f..f5a5f1c9697 100644
> --- a/gcc/analyzer/svalue.cc
> +++ b/gcc/analyzer/svalue.cc
> @@ -96,7 +96,7 @@ json::value *
> svalue::to_json () const
> {
> label_text desc = get_desc (true);
> - json::value *sval_js = new json::string (desc.m_buffer);
> + json::value *sval_js = new json::string (desc.get ());
> return sval_js;
> }
>
> diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
> index 2faed0c1607..68b94da40cc 100644
> --- a/gcc/c-family/c-format.cc
> +++ b/gcc/c-family/c-format.cc
> @@ -4617,14 +4617,14 @@ class range_label_for_format_type_mismatch
> label_text get_text (unsigned range_idx) const final override
> {
> label_text text = range_label_for_type_mismatch::get_text
> (range_idx);
> - if (text.m_buffer == NULL)
> + if (text.get () == NULL)
> return text;
>
> indirection_suffix suffix (m_pointer_count);
> char *p = (char *) alloca (suffix.get_buffer_size ());
> suffix.fill_buffer (p);
>
> - char *result = concat (text.m_buffer, p, NULL);
> + char *result = concat (text.get (), p, NULL);
> return label_text::take (result);
> }
>
> diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-
> json.cc
> index 872c67e53ed..baadc4b27c9 100644
> --- a/gcc/diagnostic-format-json.cc
> +++ b/gcc/diagnostic-format-json.cc
> @@ -102,8 +102,8 @@ json_from_location_range (diagnostic_context
> *context,
> if (loc_range->m_label)
> {
> label_text text (loc_range->m_label->get_text (range_idx));
> - if (text.m_buffer)
> - result->set ("label", new json::string (text.m_buffer));
> + if (text.get ())
> + result->set ("label", new json::string (text.get ()));
> }
>
> return result;
> diff --git a/gcc/diagnostic-format-sarif.cc b/gcc/diagnostic-format-
> sarif.cc
> index 1e4ebc8ad38..fc28d160c38 100644
> --- a/gcc/diagnostic-format-sarif.cc
> +++ b/gcc/diagnostic-format-sarif.cc
> @@ -582,7 +582,7 @@ sarif_builder::make_location_object (const
> diagnostic_event &event)
>
> /* "message" property (SARIF v2.1.0 section 3.28.5). */
> label_text ev_desc = event.get_desc (false);
> - json::object *message_obj = make_message_object
> (ev_desc.m_buffer);
> + json::object *message_obj = make_message_object (ev_desc.get ());
> location_obj->set ("message", message_obj);
>
> return location_obj;
> diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-
> locus.cc
> index 08dab20e6cd..bcaa52ec779 100644
> --- a/gcc/diagnostic-show-locus.cc
> +++ b/gcc/diagnostic-show-locus.cc
> @@ -1877,9 +1877,9 @@ struct pod_label_text
> {}
>
> pod_label_text (label_text &&other)
> - : m_buffer (other.m_buffer), m_caller_owned (other.m_owned)
> + : m_buffer (other.get ()), m_caller_owned (other.is_owner ())
> {
> - other.moved_from ();
> + other.release ();
> }
>
> void maybe_free ()
> @@ -1963,7 +1963,7 @@ layout::print_any_labels (linenum_type row)
> /* Allow for labels that return NULL from their get_text
> implementation (so e.g. such labels can control their own
> visibility). */
> - if (text.m_buffer == NULL)
> + if (text.get () == NULL)
> continue;
>
> labels.safe_push (line_label (m_policy, i, disp_col,
> std::move (text)));
> diff --git a/gcc/tree-diagnostic-path.cc b/gcc/tree-diagnostic-
> path.cc
> index 2f297faed34..6612b617471 100644
> --- a/gcc/tree-diagnostic-path.cc
> +++ b/gcc/tree-diagnostic-path.cc
> @@ -61,11 +61,11 @@ class path_label : public range_label
> is special-cased for diagnostic paths. */
> bool colorize = pp_show_color (global_dc->printer);
> label_text event_text (event.get_desc (colorize));
> - gcc_assert (event_text.m_buffer);
> + gcc_assert (event_text.get ());
> pretty_printer pp;
> pp_show_color (&pp) = pp_show_color (global_dc->printer);
> diagnostic_event_id_t event_id (event_idx);
> - pp_printf (&pp, "%@ %s", &event_id, event_text.m_buffer);
> + pp_printf (&pp, "%@ %s", &event_id, event_text.get ());
> label_text result = label_text::take (xstrdup (pp_formatted_text
> (&pp)));
> return result;
> }
> @@ -173,7 +173,7 @@ struct event_range
> diagnostic_event_id_t event_id (i);
> label_text event_text (iter_event.get_desc (true));
> pretty_printer *pp = dc->printer;
> - pp_printf (pp, " %@: %s", &event_id,
> event_text.m_buffer);
> + pp_printf (pp, " %@: %s", &event_id, event_text.get ());
> pp_newline (pp);
> }
> return;
> @@ -459,7 +459,7 @@ default_tree_diagnostic_path_printer
> (diagnostic_context *context,
> {
> const diagnostic_event &event = path->get_event (i);
> label_text event_text (event.get_desc (false));
> - gcc_assert (event_text.m_buffer);
> + gcc_assert (event_text.get ());
> diagnostic_event_id_t event_id (i);
> if (context->show_path_depths)
> {
> @@ -471,17 +471,17 @@ default_tree_diagnostic_path_printer
> (diagnostic_context *context,
> if (fndecl)
> inform (event.get_location (),
> "%@ %s (fndecl %qD, depth %i)",
> - &event_id, event_text.m_buffer,
> + &event_id, event_text.get (),
> fndecl, stack_depth);
> else
> inform (event.get_location (),
> "%@ %s (depth %i)",
> - &event_id, event_text.m_buffer,
> + &event_id, event_text.get (),
> stack_depth);
> }
> else
> inform (event.get_location (),
> - "%@ %s", &event_id, event_text.m_buffer);
> + "%@ %s", &event_id, event_text.get ());
> }
> }
> break;
> @@ -519,7 +519,7 @@ default_tree_make_json_for_path
> (diagnostic_context *context,
> json_from_expanded_location (context,
>
> event.get_location ()));
> label_text event_text (event.get_desc (false));
> - event_obj->set ("description", new json::string
> (event_text.m_buffer));
> + event_obj->set ("description", new json::string
> (event_text.get ()));
> if (tree fndecl = event.get_fndecl ())
> {
> const char *function
> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index c434a246b13..193fec0a45f 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -1851,7 +1851,7 @@ public:
> label_text (label_text &&other)
> : m_buffer (other.m_buffer), m_owned (other.m_owned)
> {
> - other.moved_from ();
> + other.release ();
> }
>
> /* Move assignment. */
> @@ -1861,7 +1861,7 @@ public:
> free (m_buffer);
> m_buffer = other.m_buffer;
> m_owned = other.m_owned;
> - other.moved_from ();
> + other.release ();
> return *this;
> }
>
> @@ -1882,25 +1882,26 @@ public:
> return label_text (buffer, true);
> }
>
> - /* Take ownership of the buffer, copying if necessary. */
> - char *take_or_copy ()
> - {
> - if (m_owned)
> - return m_buffer;
> - else
> - return xstrdup (m_buffer);
> - }
> -
> - void moved_from ()
> + void release ()
> {
> m_buffer = NULL;
> m_owned = false;
> }
>
> + char *get () const
> + {
> + return m_buffer;
> + }
> +
> + bool is_owner () const
> + {
> + return m_owned;
> + }
> +
> +private:
> char *m_buffer;
> bool m_owned;
>
> -private:
> label_text (char *buffer, bool owned)
> : m_buffer (buffer), m_owned (owned)
> {}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libcpp: Improve encapsulation of label_text
2022-07-14 21:30 ` David Malcolm
@ 2022-07-15 8:44 ` Jonathan Wakely
0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Wakely @ 2022-07-15 8:44 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches
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;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-15 8:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-14 21:10 [PATCH] libcpp: Improve encapsulation of label_text Jonathan Wakely
2022-07-14 21:30 ` David Malcolm
2022-07-15 8:44 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).