[PATCH] D56206: Introduce `RemoteAddressSpaceView` and `VMReadContext`.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked 6 inline comments as done.
delcypher added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:31
+  void* config;
+  ProcessHandle target_process;
+  bool is_local;
----------------
delcypher wrote:
> vitalybuka wrote:
> > historically sanitizer common uses Google C++ naming convention so members need to have _ suffux.
> >  
> I'll try to fix that.
Fixed.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:73
+  // process in which it is being called from.
+  bool IsLocal() const { return is_local; }
+
----------------
delcypher wrote:
> vitalybuka wrote:
> > delcypher wrote:
> > > vitalybuka wrote:
> > > > If you can calculate it from target_process please remove member and simplify state
> > > We cannot calculate it. It needs to be part of the sate.
> > >> We cannot calculate it. It needs to be part of the sate.
> > I read this as:
> > ```
> > bool IsLocal() const {
> >   return config_ == nullptr || mach_task_self() == target_process_;
> > }
> > 
> > ```
> >   bool IsLocal() const {
> >    return config_ == nullptr || mach_task_self() == target_process_;
> >   }
> 
> Oh I see. I thought you where talking about `GetTargetProcess()`. However in this latest revision `IsLocal()` has actually been removed because my  internal implementation never called it.
> 
> I can probably bring it back though.
I've brought the implementation back and removed the `is_local_` field.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:17
+#define SANITIZER_VM_READ_CONTEXT_H
+#include "sanitizer_internal_defs.h"
+#include "sanitizer_platform.h"
----------------
vitalybuka wrote:
> empty live between define and include
Fixed.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_mac.cc:29
+    : config_(config), is_local_(false), target_process_(target_process) {
+  if (config_ == nullptr || mach_task_self() == target_process) {
+    is_local_ = true;
----------------
delcypher wrote:
> vitalybuka wrote:
> > if we keep it please remove {} for consistency 
> Ok. I'll fix this.
Done.


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56206/new/

https://reviews.llvm.org/D56206