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.

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

TODO/XXX/FIXME comments in src directory #4641

Closed
Trott opened this issue Jan 12, 2016 · 20 comments
Closed

TODO/XXX/FIXME comments in src directory #4641

Trott opened this issue Jan 12, 2016 · 20 comments
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.

Comments

@Trott
Copy link
Member

Trott commented Jan 12, 2016

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

@Trott Trott added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. test Issues and PRs related to the tests. good first issue Issues that are suitable for first-time contributors. lib / src Issues and PRs related to general changes in the lib or src directory. and removed test Issues and PRs related to the tests. labels Jan 12, 2016
@thefourtheye
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2016

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

@targos
Copy link
Member

targos commented Jan 12, 2016

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

@indutny
Copy link
Member

indutny commented Jan 12, 2016

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

@trevnorris
Copy link
Contributor

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

@ChALkeR
Copy link
Member

ChALkeR commented Feb 9, 2016

@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.

danbev added a commit to danbev/node that referenced this issue Jun 20, 2016
This commit attempts to fix one of the items in
nodejs#4641, which was to remove a TODO
comment from env.h regarding the naming of the ares_task_t struct.

Also, the struct ares_task_list was renamed to node_ares_task_list
following the same reasoning that is does not belong to the c-ares API.
danbev added a commit to danbev/node that referenced this issue Jun 23, 2016
This commit attempts to fix one of the items in
nodejs#4641, which was to move
dispatch_debug_messages_async to the Environment class.
jasnell pushed a commit that referenced this issue Jun 27, 2016
This commit attempts to fix one of the items in
#4641, which was to remove a TODO
comment from env.h regarding the naming of the ares_task_t struct.

Also, the struct ares_task_list was renamed to node_ares_task_list
following the same reasoning that is does not belong to the c-ares API.

PR-URL: #7345
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@danbev
Copy link
Contributor

danbev commented Jun 28, 2016

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.
}

I took a look at this and passing nullptr might be considered for libuv v2.

danbev added a commit to danbev/node that referenced this issue Jul 5, 2016
One of the issues in nodejs#4641 concerns OnConnection in pipe_wrap and
tcp_wrap which are very similar with some minor difference in how
they are coded. This commit extracts OnConnection so both these
classes can share the same implementation.
danbev added a commit to danbev/node that referenced this issue Apr 12, 2017
This commit attempts to address one of the TODOs in
nodejs#4641 regarding making the
AtExit callback's per environment, instead of the current global.

bnoordhuis provided a few options for solving this, and one was to
use a thread-local which is what this commit attempts to do.
danbev added a commit to danbev/node that referenced this issue Apr 12, 2017
This commit attempts to address one of the TODOs in
nodejs#4641 regarding making the
AtExit callback's per environment, instead of the current global.

bnoordhuis provided a few options for solving this, and one was to
use a thread-local which is what this commit attempts to do.

PR-URL: nodejs#9163
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue Apr 17, 2017
Issue 4641 contains a FIXME regarding the InitCrypto function. After
discussing this with bnoordhuis it seems to be an outdated comment.

Refs: #4641
PR-URL: #11669
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 19, 2017
Issue 4641 contains a FIXME regarding the InitCrypto function. After
discussing this with bnoordhuis it seems to be an outdated comment.

Refs: #4641
PR-URL: #11669
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danbev added a commit to danbev/node that referenced this issue Apr 20, 2017
This commit attempts to take care of a TODO reported in issue 4641.
I'm not sure if just removing errno is an option, as there might be
code that is depending on this errno being availble. If it cannot be
removed perhaps the TODO can. Hopefully feedback on this commit will
sort that out.

Refs: nodejs#4641
jasnell pushed a commit that referenced this issue Apr 21, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: #12536
Ref: #4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: #12536
Ref: #4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott removed the good first issue Issues that are suitable for first-time contributors. label Apr 25, 2017
evanlucas pushed a commit that referenced this issue May 1, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: #12536
Ref: #4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
danbev added a commit to danbev/node that referenced this issue May 2, 2017
This commit attempts to address one of the TODOs in
nodejs#4641 regarding making the
AtExit callback's per environment, instead of the current global.

bnoordhuis provided a few options for solving this, and one was to
use a thread-local which is what this commit attempts to do.

PR-URL: nodejs#9163
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
evanlucas pushed a commit that referenced this issue May 2, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: #12536
Ref: #4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue May 16, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: #12536
Ref: #4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 18, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: #12536
Ref: #4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented May 30, 2017

@Trott ... any reason to keep this open?

@Trott
Copy link
Member Author

Trott commented May 30, 2017

(copy/pasted from a related issue)

@jasnell I'm OK with closing this and the other TODO/XXX/FIXME issues. If any of those items are things that really ought to be fixed (rather than a wishlist or a "will fix after Magical Feature X is available"), a separate issue should be opened anyway because it's just getting lost in these out-of-date tracking issues.

While I think this issue is superfluous personally, anyone else should feel free to re-open (if GitHub permits them to) or comment requesting this be re-opened.

@Trott Trott closed this as completed May 30, 2017
@Trott Trott removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. mentor-available labels May 30, 2017
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Issue 4641 contains a FIXME regarding the InitCrypto function. After
discussing this with bnoordhuis it seems to be an outdated comment.

Refs: nodejs/node#4641
PR-URL: nodejs/node#11669
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
This commit removes a TODO regarding the removal of uv errno. errno
is currently used and cannot be removed so removing the comment to
avoid any confusion.

PR-URL: nodejs/node#12536
Ref: nodejs/node#4641
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

No branches or pull requests