Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor all attribute functions in rust-privacy-reporter.cc #3339

Closed
wants to merge 1 commit into from
Closed

refactor all attribute functions in rust-privacy-reporter.cc #3339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 12 additions & 21 deletions gcc/rust/checks/errors/privacy/rust-privacy-reporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "rust-hir-item.h"
#include "rust-attribute-values.h"
#include "rust-immutable-name-resolution-context.h"
#include "rust-attributes.h"

namespace Rust {
namespace Privacy {
Expand All @@ -34,33 +35,23 @@ PrivacyReporter::PrivacyReporter (
current_module (tl::nullopt)
{}

// Find a proc_macro, proc_macro_derive or proc_macro_attribute
// attribute in a vector of attribute
static tl::optional<std::string>
find_proc_macro_attribute (const AST::AttrVec &outer_attrs)
{
for (auto &a : outer_attrs)
{
auto &segments = a.get_path ().get_segments ();
if (segments.size () != 1)
continue;
auto name = segments.at (0).get_segment_name ();
if (name == Values::Attributes::PROC_MACRO
|| name == Values::Attributes::PROC_MACRO_ATTRIBUTE
|| name == Values::Attributes::PROC_MACRO_DERIVE)
return name;
}

return tl::nullopt;
}

// Common check on crate items when dealing with 'proc-macro' crate type.
static void
proc_macro_privacy_check (std::unique_ptr<HIR::Item> &item)
{
if (item->get_hir_kind () == HIR::Node::BaseKind::VIS_ITEM)
{
auto attribute = find_proc_macro_attribute (item->get_outer_attrs ());
tl::optional<std::string> attribute = tl::nullopt;
// Find a proc_macro, proc_macro_derive or proc_macro_attribute
// attribute in a vector of attribute
for (const auto &attr : item->get_outer_attrs ())
{
if (Analysis::Attributes::is_proc_macro_type (attr) != tl::nullopt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems odd i think you should be storing the result here in a variable instead of calling it twice like this

{
attribute = Analysis::Attributes::is_proc_macro_type (attr);
break;
}
}

bool pub_item = static_cast<HIR::VisItem *> (item.get ())
->get_visibility ()
Expand Down
21 changes: 12 additions & 9 deletions gcc/rust/util/rust-attributes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,27 +223,30 @@ check_doc_attribute (const AST::Attribute &attribute)
}
}
}

static bool
is_proc_macro_type (const AST::Attribute &attribute)
tl::optional<std::string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we return an enum instead of a string here ? Enums are cheap, strings are not. and if I'm reading this correctly string values are known statically.

Attributes::is_proc_macro_type (const AST::Attribute &attribute)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're changing the semantic of the function, either you need a new one (two different functions) or you need to rename this one, we're not deciding if an attribute is or is not a proc macro anymore but returning the kind of proc macro with your new code.

I'd keep two different functions, even if one is calling the other and simply using tl::optional::has_value()

{
BuiltinAttrDefinition result;
if (!is_builtin (attribute, result))
return false;
return tl::nullopt;

auto name = result.name;
return name == Attrs::PROC_MACRO || name == Attrs::PROC_MACRO_DERIVE
|| name == Attrs::PROC_MACRO_ATTRIBUTE;
if (name == Attrs::PROC_MACRO || name == Attrs::PROC_MACRO_DERIVE
|| name == Attrs::PROC_MACRO_ATTRIBUTE)
{
return name;
}
else
return tl::nullopt;
}

// Emit an error when one encountered attribute is either #[proc_macro],
// #[proc_macro_attribute] or #[proc_macro_derive]
static void
check_proc_macro_non_function (const AST::AttrVec &attributes)
{
for (auto &attr : attributes)
{
if (is_proc_macro_type (attr))
if (Attributes::is_proc_macro_type (attr) != tl::nullopt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this not just be if (Attributes::is_proc_macro_type (attr)) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it could be shortened.

rust_error_at (
attr.get_locus (),
"the %<#[%s]%> attribute may only be used on bare functions",
Expand All @@ -258,7 +261,7 @@ check_proc_macro_non_root (AST::AttrVec attributes, location_t loc)
{
for (auto &attr : attributes)
{
if (is_proc_macro_type (attr))
if (Attributes::is_proc_macro_type (attr) != tl::nullopt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

{
rust_error_at (
loc,
Expand Down
2 changes: 2 additions & 0 deletions gcc/rust/util/rust-attributes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class Attributes
{
public:
static bool is_known (const std::string &attribute_path);
static tl::optional<std::string>
is_proc_macro_type (const AST::Attribute &attribute);
};

enum CompilerPass
Expand Down
Loading