TODO/XXX/FIXME comments in src directory

[Trott]

Ref: #264

There are 40+ TODO, XXX, and FIXME comments in the src directory. It would be great to either remove them from the code (if they are no longer valid or at least not particularly high value), or get issues opened for them, or just get whatever it is they are addressing addressed. Here they are as of this writing.

src/debug-agent.cc:

    // Waiting for client, do not send anything just yet
    // TODO(indutny): move this to js-land
    if (a->wait_) {
      a->messages_.PushFront(msg);  // Push message back into the ready queue.

src/debug-agent.h:

  };

  // TODO(indutny): Verify that there are no races
  State state_;

src/env-inl.h:

    const v8::PropertyCallbackInfo<T>& info) {
  ASSERT(info.Data()->IsExternal());
  // XXX(bnoordhuis) Work around a g++ 4.9.2 template type inferrer bug
  // when the expression is written as info.Data().As<v8::External>().
  v8::Local<v8::Value> data = info.Data();

src/env.h:

class Environment;

// TODO(bnoordhuis) Rename struct, the ares_ prefix implies it's part
// of the c-ares API while the _t suffix implies it's a typedef.
struct ares_task_t {

src/node.cc:

static void IdleImmediateDummy(uv_idle_t* handle) {
  // Do nothing. Only for maintaining event loop.
  // TODO(bnoordhuis) Maybe make libuv accept nullptr idle callbacks.
}

...
  Local<String> path_string;
  if (path != nullptr) {
    // FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8.
    path_string = String::NewFromUtf8(env->isolate(), path);
  }
...
  Local<Object> e = Exception::Error(js_msg)->ToObject(isolate);

  // TODO(piscisaureus) errno should probably go; the user has no way of
  // knowing which uv errno value maps to which error.
  e->Set(env->errno_string(), Integer::New(isolate, errorno));
...
  bool has_domain = false;

  // TODO(trevnorris): Adding "_asyncQueue" to the "this" in the init callback
  // is a horrible way to detect usage. Rethink how detection should happen.
  if (recv->IsObject()) {
...
// Used to load 'module.node' dynamically shared objects.
//
// FIXME(bnoordhuis) Not multi-context ready. TBD how to resolve the conflict
// when two contexts try to load the same shared object. Maybe have a shadow
// cache that's a plain C list or hash table that's shared across contexts?
...
void FatalException(Isolate* isolate, const TryCatch& try_catch) {
  HandleScope scope(isolate);
  // TODO(bajtos) do not call FatalException if try_catch is verbose
  // (requires V8 API to expose getter for try_catch.is_verbose_)
  FatalException(isolate, try_catch.Exception(), try_catch.Message());
...
                               const PropertyCallbackInfo<void>& info) {
  node::Utf8Value title(info.GetIsolate(), value);
  // TODO(piscisaureus): protect with a lock
  uv_set_process_title(*title);
}
...

  obj->Set(env->uv_string(), True(env->isolate()));
  // TODO(bnoordhuis) ping libuv
  obj->Set(env->ipv6_lc_string(), True(env->isolate()));

...
// Called from V8 Debug Agent TCP thread.
static void DispatchMessagesDebugAgentCallback(Environment* env) {
  // TODO(indutny): move async handle to environment
  uv_async_send(&dispatch_debug_messages_async);
}
...

static int RegisterDebugSignalHandler() {
  // FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
  RegisterSignalHandler(SIGUSR1, EnableDebugSignalHandler);
  // Unblock SIGUSR1.  A pending SIGUSR1 signal will now be delivered.
...
  ParseArgs(argc, argv, exec_argc, exec_argv, &v8_argc, &v8_argv);

  // TODO(bnoordhuis) Intercept --prof arguments and start the CPU profiler
  // manually?  That would give us a little more control over its runtime
  // behavior but it could also interfere with the user's intentions in ways
...


// TODO(bnoordhuis) Turn into per-context event.
void RunAtExit(Environment* env) {
  AtExitCallback* p = at_exit_functions_;
...
  // epoll_wait() and friends so profiling tools can filter it out.  The samples
  // still end up in v8.log but with state=IDLE rather than state=EXTERNAL.
  // TODO(bnoordhuis) Depends on a libuv implementation detail that we should
  // probably fortify in the API contract, namely that the last started prepare
  // or check watcher runs first.  It's not 100% foolproof; if an add-on starts

src/node.h:

#ifdef _WIN32
// TODO(tjfontaine) consider changing the usage of ssize_t to ptrdiff_t
#if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED)
typedef intptr_t ssize_t;

src/node.js:

            })) {

          // XXX Fix this terrible hack!
          //
          // Give the client program a few ticks to connect.
...
          if (!process.emit('unhandledRejection', reason, promise)) {
            // Nobody is listening.
            // TODO(petkaantonov) Take some default action, see #830
          } else {
            hadListeners = true;
...
      // Load tcp_wrap to avoid situation where we might immediately receive
      // a message.
      // FIXME is this really necessary?
      process.binding('tcp_wrap');

src/node_contextify.cc:

  // XXX(isaacs): This function only exists because of a shortcoming of
  // the V8 SetNamedPropertyHandler function.
  //

src/node_crypto.cc:

    char fingerprint[EVP_MAX_MD_SIZE * 3];

    // TODO(indutny): Unify it with buffer's code
    for (i = 0; i < md_size; i++) {
      fingerprint[3*i] = hex[(md[i] & 0xf0) >> 4];
...


// TODO(indutny): Split it into multiple smaller functions
template <class Base>
void SSLWrap<Base>::GetPeerCertificate(
...
  Base* w = Unwrap<Base>(args.Holder());

  // XXX(bnoordhuis) The UNABLE_TO_GET_ISSUER_CERT error when there is no
  // peer certificate is questionable but it's compatible with what was
  // here before.
...
    return args.GetReturnValue().SetNull();

  // XXX(bnoordhuis) X509_verify_cert_error_string() is not actually thread-safe
  // in the presence of invalid error codes.  Probably academical but something
  // to keep in mind if/when node ever grows multi-isolate capabilities.
...
    CHECK(err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL);

    // XXX We need to drain the error queue for this thread or else OpenSSL
    // has the possibility of blocking connections? This problem is not well
    // understood. And we should be somehow propagating these errors up
...
  Environment* env = Environment::GetCurrent(args);

  // TODO(indutny): Support raw curves?
  CHECK(args[0]->IsString());
  node::Utf8Value curve(env->isolate(), args[0]);
...
  if (args[5]->IsFunction()) {
    obj->Set(env->ondone_string(), args[5]);
    // XXX(trevnorris): This will need to go with the rest of domains.
    if (env->in_domain())
      obj->Set(env->domain_string(), env->domain_array()->Get(0));
...
  if (args[1]->IsFunction()) {
    obj->Set(FIXED_ONE_BYTE_STRING(args.GetIsolate(), "ondone"), args[1]);
    // XXX(trevnorris): This will need to go with the rest of domains.
    if (env->in_domain())
      obj->Set(env->domain_string(), env->domain_array()->Get(0));
...


// FIXME(bnoordhuis) Handle global init correctly.
void InitCrypto(Local<Object> target,
                Local<Value> unused,

src/node_crypto_bio.cc:

  bio->ptr = new NodeBIO();

  // XXX Why am I doing it?!
  bio->shutdown = 1;
  bio->init = 1;

src/node_file.cc:

  int fd = args[0]->Int32Value();

  // FIXME(bnoordhuis) It's questionable to reject non-ints here but still
  // allow implicit coercion from null or undefined to zero.  Probably best
  // handled in lib/fs.js.

src/node_http_parser.cc:

    else if (on_heap_ || str_ + size_ != str) {
      // Non-consecutive input, make a copy on the heap.
      // TODO(bnoordhuis) Use slab allocation, O(n) allocs is bad.
      char* s = new char[size_ + size];
      memcpy(s, str_, size_);
...
    Local<Integer> nparsed_obj = Integer::New(env()->isolate(), nparsed);
    // If there was a parse error in one of the callbacks
    // TODO(bnoordhuis) What if there is an error on EOF?
    if (!parser_.upgrade && nparsed != len) {
      enum http_errno err = HTTP_PARSER_ERRNO(&parser_);

src/pipe_wrap.cc:

// TODO(bnoordhuis) share with TCPWrap?
class PipeConnectWrap : public ReqWrap<uv_connect_t> {
 public:
...


// TODO(bnoordhuis) maybe share with TCPWrap?
void PipeWrap::OnConnection(uv_stream_t* handle, int status) {
  PipeWrap* pipe_wrap = static_cast<PipeWrap*>(handle->data);
...
}

// TODO(bnoordhuis) Maybe share this with TCPWrap?
void PipeWrap::AfterConnect(uv_connect_t* req, int status) {
  PipeConnectWrap* req_wrap = static_cast<PipeConnectWrap*>(req->data);

src/process_wrap.cc:

    }

    // TODO(bnoordhuis) is this possible to do without mallocing ?

    // options.file

src/req-wrap-inl.h:

    object->Set(env->domain_string(), env->domain_array()->Get(0));

  // FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is
  // arguably a good indicator that there should be more than one queue.
  env->req_wrap_queue()->PushBack(reinterpret_cast<ReqWrap<uv_req_t>*>(this));

src/req-wrap.h:

 public:
  T req_;  // Must be last.  TODO(bnoordhuis) Make private.
};

src/string_bytes.cc:

  if (enc == HEX && string->Length() % 2 != 0)
    return false;
  // TODO(bnoordhuis) Add BASE64 check?
  return true;
}

src/string_bytes.h:

  // Does the string match the encoding? Quick but non-exhaustive.
  // Example: a HEX string must have a length that's a multiple of two.
  // FIXME(bnoordhuis) IsMaybeValidString()? Naming things is hard...
  static bool IsValidString(v8::Isolate* isolate,
                            v8::Local<v8::String> string,

src/udp_wrap.cc:

// TODO(bnoordhuis) share with StreamWrap::AfterWrite() in stream_wrap.cc
void UDPWrap::OnSend(uv_udp_send_t* req, int status) {
  SendWrap* req_wrap = static_cast<SendWrap*>(req->data);

/cc @indutny @bnoordhuis @piscisaureus @trevnorris @isaacs

[thefourtheye]

Wouldn't it be better if we had the file and line number link? :D

[Fishrock123]

line number links would be nice yeah, but these are easy enough to search for I think.

[cjihrig]

Line numbers will also get stale unless the link is tied to a specific commit.

[targos]

tip: press Y when you are viewing a file on master to put the latest commit id in the URL

[indutny]

• debug-agent feel free to clear them, doesn't make sense to me anymore
• node.cc is still relevant
• node_crypto.cc - relevant

[trevnorris]

• node.cc still working on that issue
• node_crypto.cc don't remember why those are there. can be removed.

[ChALkeR]

@Trott Please add this one to the list: src/fs_event_wrap.cc#L139:

  // We're in a bind here. libuv can set both UV_RENAME and UV_CHANGE but
  // the Node API only lets us pass a single event to JS land.
  //
  // The obvious solution is to run the callback twice, once for each event.
  // However, since the second event is not allowed to fire if the handle is
  // closed after the first event, and since there is no good way to detect
  // closed handles, that option is out.
  //
  // For now, ignore the UV_CHANGE event if UV_RENAME is also set. Make the
  // assumption that a rename implicitly means an attribute change. Not too
  // unreasonable, right? Still, we should revisit this before v1.0.
  Local<String> event_string;
  if (status) {
    event_string = String::Empty(env->isolate());
  } else if (events & UV_RENAME) {
    event_string = env->rename_string();
  } else if (events & UV_CHANGE) {
    event_string = env->change_string();
  } else {
    CHECK(0 && "bad fs events flag");
    ABORT();
  }

/cc @bnoordhuis, this mentions revisiting before v1.0.