Introduce HObjectAccess, which is used by LoadNamedField and StoreNamedField to denote what parts

src/hydrogen.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 999 matching lines @@
       HValue* object,
       HValue* key,
       HValue* val,
       HCheckMaps* mapcheck,
       bool is_js_array,
       ElementsKind elements_kind,
       bool is_store,
       KeyedAccessStoreMode store_mode,
       Representation checked_index_representation = Representation::None());
 
-  HInstruction* BuildStoreMap(HValue* object, HValue* map);
-  HInstruction* BuildStoreMap(HValue* object, Handle<Map> map);
+  HStoreNamedField* AddStore(HValue *object, ObjectAccess access, HValue *val);
+
+  ObjectAccess AccessArray(int offset);

[danno, 2013/04/26 09:39:38]

I think these methods are better suited as static methods on the ObjectAccess
class in order to encapsulate the logic mapping fields to their flags/etc. in
one class (even though it means passing the builder--or at least the isolate--as
a parameter):

ObjectAccess::ForArrayLength(this); 

[titzer, 2013/04/30 15:56:47]

I'm OK with putting the guts of those methods in ObjectAccess if they have to do
anything tricky, but as for hydrogen.cc, having these methods saves a lot of
characters, reducing the need for other intermediate build methods (e.g.
BuildStoreMap is now gone)

[danno, 2013/05/02 14:16:47]

Tricky/non-trick is an arbitrary judgment call, I don't think encapsulation
is... and it's binary. I feel pretty strongly that if we're going through all
this trouble to create a class to hide the details of mapping ObjectAccess to
portions/flags/whatever, all of the logic for that should be in on place... the
ObjectAccess class and not partially sprinkled through some methods on
HGraphBuilder. I don't think there should be any reason to expose the "portion"
of the ObjectAccess in the public API.

Also, "saves a lot of characters" doesn't seem like a very good argument for
choosing readable names. For example, I think "AccessInobject" doesn't tell me
much. AccessInobject what? Properties? If so, why not put that in the name?
Access is a verb, so does the method actually access something? I don't mean to
be pedantic, but for others reading the code for the first time, a few extra
characters can drastically improve readability if they are the right ones.


On 2013/04/30 15:56:47, titzer wrote:

[titzer, 2013/05/03 09:18:41]

I've moved these methods into static constructors in HObjectAccess. What I meant
by saving characters was that writing AccessX() within the GraphBuilder code is
shorter than writing HObjectAccess::X(zone()), because GraphBuilder already
closes over zone, and the code is already in the GraphBuilder scope. I did not
mean to argue X versus a more readable AccessX. But I'm OK with putting it all
in one place at the expense of needing HObjectAccess:: as a prefix.

+  ObjectAccess AccessArrayLength();
+  ObjectAccess AccessElements();
+  ObjectAccess AccessFixedArrayLength();
+  ObjectAccess AccessMap();
+  ObjectAccess AccessField(Handle<Map> map, Handle<String> name,

[danno, 2013/04/26 09:39:38]

Shouldn't need the name here, the LookupResult has it.

[titzer, 2013/04/30 15:56:47]

I hunted around inside the LookupResult and couldn't find it. Does it require
looking up in the map with the index result from the LookupResult?

+     LookupResult* lookup);
+  ObjectAccess AccessInobject(Handle<String> name, int offset);

[danno, 2013/04/26 09:39:38]

Should take a map here. Look up the name in the descriptor array if you need it.

[titzer, 2013/04/30 15:56:47]

We might need another version that takes a map when we move the ObjectAccess to
use maps.

 
   class IfBuilder {
    public:
     explicit IfBuilder(HGraphBuilder* builder,
                        int position = RelocInfo::kNoPosition);
     IfBuilder(HGraphBuilder* builder,
               HIfContinuation* continuation);
 
     ~IfBuilder() {
       if (!finished_) End();
@@ skipping 916 matching lines @@
   EmbeddedVector<char, 64> filename_;
   HeapStringAllocator string_allocator_;
   StringStream trace_;
   int indent_;
 };
 
 
 } }  // namespace v8::internal
 
 #endif  // V8_HYDROGEN_H_