Commit 628431b2 authored by Sigurd Schneider's avatar Sigurd Schneider Committed by Commit Bot

[torque] Generate non-object field accessors from torque

Bug: v8:7793
Change-Id: Ic485dc953c80d055ff190b8be2a5434e5cdbeb5d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1624214
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: 's avatarTobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61828}
parent 843b6646
...@@ -73,6 +73,7 @@ extern class Context extends HeapObject { ...@@ -73,6 +73,7 @@ extern class Context extends HeapObject {
} }
type NativeContext extends Context; type NativeContext extends Context;
@generateCppClass
extern class Oddball extends HeapObject { extern class Oddball extends HeapObject {
to_number_raw: float64; to_number_raw: float64;
to_string: String; to_string: String;
...@@ -169,6 +170,7 @@ type RootIndex generates 'TNode<Int32T>' constexpr 'RootIndex'; ...@@ -169,6 +170,7 @@ type RootIndex generates 'TNode<Int32T>' constexpr 'RootIndex';
@abstract @abstract
@noVerifier @noVerifier
@generateCppClass
extern class FixedArrayBase extends HeapObject { extern class FixedArrayBase extends HeapObject {
length: Smi; length: Smi;
} }
......
...@@ -1107,7 +1107,7 @@ void JSGlobalObject::JSGlobalObjectVerify(Isolate* isolate) { ...@@ -1107,7 +1107,7 @@ void JSGlobalObject::JSGlobalObjectVerify(Isolate* isolate) {
} }
void Oddball::OddballVerify(Isolate* isolate) { void Oddball::OddballVerify(Isolate* isolate) {
TorqueGeneratedClassVerifiers::OddballVerify(*this, isolate); TorqueGeneratedOddball::OddballVerify(isolate);
Heap* heap = isolate->heap(); Heap* heap = isolate->heap();
Object number = to_number(); Object number = to_number();
if (number.IsHeapObject()) { if (number.IsHeapObject()) {
......
...@@ -18,33 +18,19 @@ ...@@ -18,33 +18,19 @@
namespace v8 { namespace v8 {
namespace internal { namespace internal {
OBJECT_CONSTRUCTORS_IMPL(Oddball, HeapObject) TQ_OBJECT_CONSTRUCTORS_IMPL(Oddball)
CAST_ACCESSOR(Oddball)
double Oddball::to_number_raw() const {
return ReadField<double>(kToNumberRawOffset);
}
void Oddball::set_to_number_raw(double value) {
WriteField<double>(kToNumberRawOffset, value);
}
void Oddball::set_to_number_raw_as_bits(uint64_t bits) { void Oddball::set_to_number_raw_as_bits(uint64_t bits) {
// Bug(v8:8875): HeapNumber's double may be unaligned. // Bug(v8:8875): HeapNumber's double may be unaligned.
WriteUnalignedValue<uint64_t>(field_address(kToNumberRawOffset), bits); WriteUnalignedValue<uint64_t>(field_address(kToNumberRawOffset), bits);
} }
ACCESSORS(Oddball, to_string, String, kToStringOffset)
ACCESSORS(Oddball, to_number, Object, kToNumberOffset)
ACCESSORS(Oddball, type_of, String, kTypeOfOffset)
byte Oddball::kind() const { byte Oddball::kind() const {
return Smi::ToInt(READ_FIELD(*this, kKindOffset)); return Smi::ToInt(TorqueGeneratedOddball::kind());
} }
void Oddball::set_kind(byte value) { void Oddball::set_kind(byte value) {
WRITE_FIELD(*this, kKindOffset, Smi::FromInt(value)); TorqueGeneratedOddball::set_kind(Smi::FromInt(value));
} }
// static // static
......
...@@ -6,7 +6,7 @@ ...@@ -6,7 +6,7 @@
#define V8_OBJECTS_ODDBALL_H_ #define V8_OBJECTS_ODDBALL_H_
#include "src/objects/heap-object.h" #include "src/objects/heap-object.h"
#include "torque-generated/field-offsets-tq.h" #include "torque-generated/class-definitions-tq.h"
// Has to be the last include (doesn't have include guards): // Has to be the last include (doesn't have include guards):
#include "src/objects/object-macros.h" #include "src/objects/object-macros.h"
...@@ -15,45 +15,26 @@ namespace v8 { ...@@ -15,45 +15,26 @@ namespace v8 {
namespace internal { namespace internal {
// The Oddball describes objects null, undefined, true, and false. // The Oddball describes objects null, undefined, true, and false.
class Oddball : public HeapObject { class Oddball : public TorqueGeneratedOddball<Oddball, HeapObject> {
public: public:
// [to_number_raw]: Cached raw to_number computed at startup. // [to_number_raw]: Cached raw to_number computed at startup.
inline double to_number_raw() const;
inline void set_to_number_raw(double value);
inline void set_to_number_raw_as_bits(uint64_t bits); inline void set_to_number_raw_as_bits(uint64_t bits);
// [to_string]: Cached to_string computed at startup.
DECL_ACCESSORS(to_string, String)
// [to_number]: Cached to_number computed at startup.
DECL_ACCESSORS(to_number, Object)
// [typeof]: Cached type_of computed at startup.
DECL_ACCESSORS(type_of, String)
inline byte kind() const; inline byte kind() const;
inline void set_kind(byte kind); inline void set_kind(byte kind);
// Oddball has a custom verifier.
void OddballVerify(Isolate* isolate);
// ES6 section 7.1.3 ToNumber for Boolean, Null, Undefined. // ES6 section 7.1.3 ToNumber for Boolean, Null, Undefined.
V8_WARN_UNUSED_RESULT static inline Handle<Object> ToNumber( V8_WARN_UNUSED_RESULT static inline Handle<Object> ToNumber(
Isolate* isolate, Handle<Oddball> input); Isolate* isolate, Handle<Oddball> input);
DECL_CAST(Oddball)
// Dispatched behavior.
DECL_VERIFIER(Oddball)
// Initialize the fields. // Initialize the fields.
static void Initialize(Isolate* isolate, Handle<Oddball> oddball, static void Initialize(Isolate* isolate, Handle<Oddball> oddball,
const char* to_string, Handle<Object> to_number, const char* to_string, Handle<Object> to_number,
const char* type_of, byte kind); const char* type_of, byte kind);
DEFINE_FIELD_OFFSET_CONSTANTS(HeapObject::kHeaderSize,
TORQUE_GENERATED_ODDBALL_FIELDS)
// TODO(v8:8989): [torque] Support marker constants.
static const int kTaggedFieldsStartOffset = kToStringOffset;
static const int kTaggedFieldsEndOffset = kKindOffset;
static const byte kFalse = 0; static const byte kFalse = 0;
static const byte kTrue = 1; static const byte kTrue = 1;
static const byte kNotBooleanMask = static_cast<byte>(~1); static const byte kNotBooleanMask = static_cast<byte>(~1);
...@@ -68,14 +49,16 @@ class Oddball : public HeapObject { ...@@ -68,14 +49,16 @@ class Oddball : public HeapObject {
static const byte kStaleRegister = 10; static const byte kStaleRegister = 10;
static const byte kSelfReferenceMarker = 10; static const byte kSelfReferenceMarker = 10;
using BodyDescriptor = FixedBodyDescriptor<kTaggedFieldsStartOffset, static_assert(kStartOfWeakFieldsOffset == kEndOfWeakFieldsOffset,
kTaggedFieldsEndOffset, kSize>; "Ensure BodyDescriptor does not need to handle weak fields.");
using BodyDescriptor = FixedBodyDescriptor<kStartOfStrongFieldsOffset,
kEndOfStrongFieldsOffset, kSize>;
STATIC_ASSERT(kKindOffset == Internals::kOddballKindOffset); STATIC_ASSERT(kKindOffset == Internals::kOddballKindOffset);
STATIC_ASSERT(kNull == Internals::kNullOddballKind); STATIC_ASSERT(kNull == Internals::kNullOddballKind);
STATIC_ASSERT(kUndefined == Internals::kUndefinedOddballKind); STATIC_ASSERT(kUndefined == Internals::kUndefinedOddballKind);
OBJECT_CONSTRUCTORS(Oddball, HeapObject); TQ_OBJECT_CONSTRUCTORS(Oddball)
}; };
} // namespace internal } // namespace internal
......
...@@ -3004,7 +3004,11 @@ class CppClassGenerator { ...@@ -3004,7 +3004,11 @@ class CppClassGenerator {
private: private:
void GenerateClassConstructors(); void GenerateClassConstructors();
void GenerateClassFieldAccessor(const Field& f); void GenerateFieldAccessor(const Field& f);
void GenerateFieldAccessorForUntagged(const Field& f);
void GenerateFieldAccessorForSmi(const Field& f);
void GenerateFieldAccessorForObject(const Field& f);
void GenerateClassCasts(); void GenerateClassCasts();
const ClassType* type_; const ClassType* type_;
...@@ -3031,7 +3035,7 @@ void CppClassGenerator::GenerateClass() { ...@@ -3031,7 +3035,7 @@ void CppClassGenerator::GenerateClass() {
hdr_ << "public: \n"; hdr_ << "public: \n";
hdr_ << " using Super = P;\n"; hdr_ << " using Super = P;\n";
for (const Field& f : type_->fields()) { for (const Field& f : type_->fields()) {
GenerateClassFieldAccessor(f); GenerateFieldAccessor(f);
} }
GenerateClassCasts(); GenerateClassCasts();
...@@ -3109,27 +3113,123 @@ void CppClassGenerator::GenerateClassConstructors() { ...@@ -3109,27 +3113,123 @@ void CppClassGenerator::GenerateClassConstructors() {
} }
// TODO(sigurds): Keep in sync with DECL_ACCESSORS and ACCESSORS macro. // TODO(sigurds): Keep in sync with DECL_ACCESSORS and ACCESSORS macro.
void CppClassGenerator::GenerateClassFieldAccessor(const Field& f) { void CppClassGenerator::GenerateFieldAccessor(const Field& f) {
const Type* field_type = f.name_and_type.type;
if (field_type == TypeOracle::GetVoidType()) return;
if (!f.name_and_type.type->IsSubtypeOf(TypeOracle::GetTaggedType())) {
return GenerateFieldAccessorForUntagged(f);
}
if (f.name_and_type.type->IsSubtypeOf(TypeOracle::GetSmiType())) {
return GenerateFieldAccessorForSmi(f);
}
if (f.name_and_type.type->IsSubtypeOf(TypeOracle::GetObjectType())) {
return GenerateFieldAccessorForObject(f);
}
Error("Generation of field accessor for ", type_->name(),
":: ", f.name_and_type.name, " : ", *field_type, " is not supported.")
.Position(f.pos);
}
void CppClassGenerator::GenerateFieldAccessorForUntagged(const Field& f) {
DCHECK(!f.name_and_type.type->IsSubtypeOf(TypeOracle::GetTaggedType()));
const Type* field_type = f.name_and_type.type;
if (field_type == TypeOracle::GetVoidType()) return;
const Type* constexpr_version = field_type->ConstexprVersion();
if (!constexpr_version) {
Error("Field accessor for ", type_->name(), ":: ", f.name_and_type.name,
" cannot be generated because its type ", *field_type,
" is neither a subclass of Object nor does the type have a constexpr "
"version.")
.Position(f.pos);
return;
}
const std::string& name = f.name_and_type.name;
const std::string type = constexpr_version->GetGeneratedTypeName();
const std::string offset = "k" + CamelifyString(name) + "Offset";
// Generate declarations in header.
hdr_ << " inline " << type << " " << name << "() const;\n";
hdr_ << " inline void set_" << name << "(" << type << " value);\n\n";
// Generate implementation in inline header.
inl_ << "template <class D, class P>\n";
inl_ << type << " " << gen_name_ << "<D, P>::" << name << "() const {\n";
inl_ << " return this->template ReadField<" << type << ">(" << offset
<< ");\n";
inl_ << "}\n";
inl_ << "template <class D, class P>\n";
inl_ << "void " << gen_name_ << "<D, P>::set_" << name << "(" << type
<< " value) {\n";
inl_ << " this->template WriteField<" << type << ">(" << offset
<< ", value);\n";
inl_ << "}\n\n";
}
void CppClassGenerator::GenerateFieldAccessorForSmi(const Field& f) {
DCHECK(f.name_and_type.type->IsSubtypeOf(TypeOracle::GetSmiType()));
const std::string type = "Smi";
const std::string& name = f.name_and_type.name; const std::string& name = f.name_and_type.name;
const std::string& type = f.name_and_type.type->GetGeneratedTNodeTypeName(); const std::string offset = "k" + CamelifyString(name) + "Offset";
DCHECK(!f.name_and_type.type->IsSubtypeOf(TypeOracle::GetSmiType()));
// Generate declarations in header.
hdr_ << " inline " << type << " " << name << "() const;\n"; hdr_ << " inline " << type << " " << name << "() const;\n";
hdr_ << " inline void set_" << name << "(" << type << " value,\n"; hdr_ << " inline void set_" << name << "(" << type << " value);\n\n";
hdr_ << " WriteBarrierMode mode = UPDATE_WRITE_BARRIER);\n\n";
const std::string offset = // Generate implementation in inline header.
"k" + CamelifyString(f.name_and_type.name) + "Offset";
inl_ << "template <class D, class P>\n"; inl_ << "template <class D, class P>\n";
inl_ << " " << type << " " << gen_name_ << "<D, P>::" << name inl_ << type << " " << gen_name_ << "<D, P>::" << name << "() const {\n";
<< "() const {\n"; inl_ << " return Smi::cast(READ_FIELD(*this, " << offset << "));\n";
inl_ << " " << type << " value = " << type << "::cast(READ_FIELD(*this, "
<< offset << "));\n";
inl_ << " return value;\n";
inl_ << "}\n"; inl_ << "}\n";
inl_ << "template <class D, class P>\n";
inl_ << "void " << gen_name_ << "<D, P>::set_" << name << "(" << type
<< " value) {\n";
inl_ << " DCHECK(value.IsSmi());\n";
inl_ << " WRITE_FIELD(*this, " << offset << ", value);\n";
inl_ << "}\n\n";
}
void CppClassGenerator::GenerateFieldAccessorForObject(const Field& f) {
const Type* field_type = f.name_and_type.type;
DCHECK(field_type->IsSubtypeOf(TypeOracle::GetObjectType()));
const std::string& name = f.name_and_type.name;
const std::string offset = "k" + CamelifyString(name) + "Offset";
const ClassType* class_type = ClassType::DynamicCast(field_type);
std::string type = class_type ? class_type->name() : "Object";
// Generate declarations in header.
if (!class_type && field_type != TypeOracle::GetObjectType()) {
hdr_ << " // Torque type: " << field_type->ToString() << "\n";
}
hdr_ << " inline " << type << " " << name << "() const;\n";
hdr_ << " inline void set_" << name << "(" << type
<< " value, WriteBarrierMode mode = UPDATE_WRITE_BARRIER);\n\n";
std::string type_check;
for (const std::string& runtime_type : field_type->GetRuntimeTypes()) {
if (!type_check.empty()) type_check += " || ";
type_check += "value.Is" + runtime_type + "()";
}
// Generate implementation in inline header.
inl_ << "template <class D, class P>\n";
inl_ << type << " " << gen_name_ << "<D, P>::" << name << "() const {\n";
inl_ << " Object value = READ_FIELD(*this, " << offset << ");\n";
if (class_type) {
inl_ << " return " << type << "::cast(value);\n";
} else {
inl_ << " DCHECK(" << type_check << ");\n";
inl_ << " return value;\n";
}
inl_ << "}\n";
inl_ << "template <class D, class P>\n"; inl_ << "template <class D, class P>\n";
inl_ << "void " << gen_name_ << "<D, P>::set_" << name << "(" << type inl_ << "void " << gen_name_ << "<D, P>::set_" << name << "(" << type
<< " value, WriteBarrierMode mode) {\n"; << " value, WriteBarrierMode mode) {\n";
inl_ << " SLOW_DCHECK(" << type_check << ");\n";
inl_ << " WRITE_FIELD(*this, " << offset << ", value);\n"; inl_ << " WRITE_FIELD(*this, " << offset << ", value);\n";
inl_ << " CONDITIONAL_WRITE_BARRIER(*this, " << offset inl_ << " CONDITIONAL_WRITE_BARRIER(*this, " << offset
<< ", value, mode);\n"; << ", value, mode);\n";
...@@ -3150,6 +3250,7 @@ void ImplementationVisitor::GenerateClassDefinitions( ...@@ -3150,6 +3250,7 @@ void ImplementationVisitor::GenerateClassDefinitions(
IncludeGuardScope header_guard(header, basename + ".h"); IncludeGuardScope header_guard(header, basename + ".h");
header << "#include \"src/objects/heap-number.h\"\n"; header << "#include \"src/objects/heap-number.h\"\n";
header << "#include \"src/objects/objects.h\"\n"; header << "#include \"src/objects/objects.h\"\n";
header << "#include \"src/objects/smi.h\"\n";
header << "#include \"torque-generated/field-offsets-tq.h\"\n"; header << "#include \"torque-generated/field-offsets-tq.h\"\n";
header << "#include <type_traits>\n\n"; header << "#include <type_traits>\n\n";
IncludeObjectMacrosScope header_macros(header); IncludeObjectMacrosScope header_macros(header);
...@@ -3158,6 +3259,7 @@ void ImplementationVisitor::GenerateClassDefinitions( ...@@ -3158,6 +3259,7 @@ void ImplementationVisitor::GenerateClassDefinitions(
IncludeGuardScope inline_header_guard(inline_header, basename + "-inl.h"); IncludeGuardScope inline_header_guard(inline_header, basename + "-inl.h");
inline_header << "#include \"torque-generated/class-definitions-tq.h\"\n\n"; inline_header << "#include \"torque-generated/class-definitions-tq.h\"\n\n";
inline_header << "#include \"src/objects/objects-inl.h\"\n\n";
IncludeObjectMacrosScope inline_header_macros(inline_header); IncludeObjectMacrosScope inline_header_macros(inline_header);
NamespaceScope inline_header_namespaces(inline_header, {"v8", "internal"}); NamespaceScope inline_header_namespaces(inline_header, {"v8", "internal"});
......
...@@ -19,11 +19,14 @@ class TypeOracle : public ContextualClass<TypeOracle> { ...@@ -19,11 +19,14 @@ class TypeOracle : public ContextualClass<TypeOracle> {
public: public:
static const AbstractType* GetAbstractType( static const AbstractType* GetAbstractType(
const Type* parent, std::string name, bool transient, const Type* parent, std::string name, bool transient,
std::string generated, const Type* non_constexpr_version) { std::string generated, const AbstractType* non_constexpr_version) {
AbstractType* result = AbstractType* result =
new AbstractType(parent, transient, std::move(name), new AbstractType(parent, transient, std::move(name),
std::move(generated), non_constexpr_version); std::move(generated), non_constexpr_version);
Get().nominal_types_.push_back(std::unique_ptr<AbstractType>(result)); Get().nominal_types_.push_back(std::unique_ptr<AbstractType>(result));
if (non_constexpr_version) {
non_constexpr_version->SetConstexprVersion(result);
}
return result; return result;
} }
......
...@@ -67,10 +67,12 @@ const AbstractType* TypeVisitor::ComputeType(AbstractTypeDeclaration* decl) { ...@@ -67,10 +67,12 @@ const AbstractType* TypeVisitor::ComputeType(AbstractTypeDeclaration* decl) {
ReportError("cannot declare a transient type that is also constexpr"); ReportError("cannot declare a transient type that is also constexpr");
} }
const Type* non_constexpr_version = nullptr; const AbstractType* non_constexpr_version = nullptr;
if (decl->is_constexpr) { if (decl->is_constexpr) {
non_constexpr_version = Declarations::LookupType( QualifiedName non_constexpr_name{GetNonConstexprName(decl->name->value)};
QualifiedName{GetNonConstexprName(decl->name->value)}); const Type* non_constexpr_type =
Declarations::LookupType(non_constexpr_name);
non_constexpr_version = AbstractType::DynamicCast(non_constexpr_type);
DCHECK_NOT_NULL(non_constexpr_version); DCHECK_NOT_NULL(non_constexpr_version);
} }
......
...@@ -341,7 +341,7 @@ void ClassType::Finalize() const { ...@@ -341,7 +341,7 @@ void ClassType::Finalize() const {
if (const ClassType* super_class = ClassType::DynamicCast(parent())) { if (const ClassType* super_class = ClassType::DynamicCast(parent())) {
if (super_class->HasIndexedField()) flags_ |= ClassFlag::kHasIndexedField; if (super_class->HasIndexedField()) flags_ |= ClassFlag::kHasIndexedField;
if (!super_class->IsAbstract() && !HasSameInstanceTypeAsParent()) { if (!super_class->IsAbstract() && !HasSameInstanceTypeAsParent()) {
Lint( Error(
"Super class must either be abstract (annotate super class with " "Super class must either be abstract (annotate super class with "
"@abstract) " "@abstract) "
"or this class must have the same instance type as the super class " "or this class must have the same instance type as the super class "
...@@ -355,13 +355,10 @@ void ClassType::Finalize() const { ...@@ -355,13 +355,10 @@ void ClassType::Finalize() const {
is_finalized_ = true; is_finalized_ = true;
if (GenerateCppClassDefinitions()) { if (GenerateCppClassDefinitions()) {
for (const Field& f : fields()) { for (const Field& f : fields()) {
const Type* field_type = f.name_and_type.type; if (f.is_weak) {
if (!field_type->IsSubtypeOf(TypeOracle::GetObjectType()) || Error("Generation of C++ class for Torque class ", name(),
field_type->IsSubtypeOf(TypeOracle::GetSmiType()) || " is not supported yet, because field ", f.name_and_type.name,
field_type->IsSubtypeOf(TypeOracle::GetNumberType())) { ": ", *f.name_and_type.type, " is a weak field.")
Lint("Generation of C++ class for Torque class ", name(),
" is not supported yet, because the type of field ",
f.name_and_type.name, " cannot be handled yet")
.Position(f.pos); .Position(f.pos);
} }
} }
......
...@@ -102,6 +102,7 @@ class V8_EXPORT_PRIVATE Type : public TypeBase { ...@@ -102,6 +102,7 @@ class V8_EXPORT_PRIVATE Type : public TypeBase {
} }
virtual bool IsTransient() const { return false; } virtual bool IsTransient() const { return false; }
virtual const Type* NonConstexprVersion() const { return this; } virtual const Type* NonConstexprVersion() const { return this; }
virtual const Type* ConstexprVersion() const { return nullptr; }
base::Optional<const ClassType*> ClassSupertype() const; base::Optional<const ClassType*> ClassSupertype() const;
virtual std::vector<std::string> GetRuntimeTypes() const { return {}; } virtual std::vector<std::string> GetRuntimeTypes() const { return {}; }
static const Type* CommonSupertype(const Type* a, const Type* b); static const Type* CommonSupertype(const Type* a, const Type* b);
...@@ -210,8 +211,16 @@ class AbstractType final : public Type { ...@@ -210,8 +211,16 @@ class AbstractType final : public Type {
const Type* NonConstexprVersion() const override { const Type* NonConstexprVersion() const override {
if (non_constexpr_version_) return non_constexpr_version_; if (non_constexpr_version_) return non_constexpr_version_;
return this; if (!IsConstexpr()) return this;
return nullptr;
} }
const AbstractType* ConstexprVersion() const override {
if (constexpr_version_) return constexpr_version_;
if (IsConstexpr()) return this;
return nullptr;
}
std::vector<std::string> GetRuntimeTypes() const override { return {name()}; } std::vector<std::string> GetRuntimeTypes() const override { return {name()}; }
private: private:
...@@ -230,12 +239,18 @@ class AbstractType final : public Type { ...@@ -230,12 +239,18 @@ class AbstractType final : public Type {
!non_constexpr_version->IsConstexpr()); !non_constexpr_version->IsConstexpr());
} }
void SetConstexprVersion(const AbstractType* type) const {
DCHECK_EQ(GetConstexprName(name()), type->name());
constexpr_version_ = type;
}
bool IsTransient() const override { return transient_; } bool IsTransient() const override { return transient_; }
bool transient_; bool transient_;
const std::string name_; const std::string name_;
const std::string generated_type_; const std::string generated_type_;
const Type* non_constexpr_version_; const Type* non_constexpr_version_;
mutable const AbstractType* constexpr_version_ = nullptr;
}; };
// For now, builtin pointers are restricted to Torque-defined builtins. // For now, builtin pointers are restricted to Torque-defined builtins.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment