[Webkit-unassigned] [Bug 53747] Add some dynamic annotations to JavaScriptCore/wtf

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53747





--- Comment #7 from Mark Rowe (bdash) <mrowe at apple.com>  2011-02-04 05:09:57 PST ---
(From update of attachment 81172)
View in context: https://bugs.webkit.org/attachment.cgi?id=81172&action=review

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:27
> + * Author: Kostya Serebryany

We don’t list author information in source files.  The version control system tracks this information.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:31
> +#define DynamicAnnotations_h

The convention in the wtf directory appears to be to use WTF_FileName_h as the include guard.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:33
> +#include "Platform.h"

This include should not be present.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:35
> +/* This file defines dynamic annotations for use with dynamic analysis

IMO the term “dynamic annotations” is far too generic to clearly communicate what it is that this file provides.  Is there some clearer term that can be used instead?

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:63
> +#define ANNOTATE_HAPPENS_AFTER(obj) WebKitAnnotateCondVarWait(__FILE__, __LINE__, obj, 0)

The naming of these two macros is confusing to me.  The names don’t appear to communicate what their purpose is.  Something happens before or after something else?  Can we come up with self-documenting names so that we don’t need to add multi-line comments at every use?

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:71
> +void WebKitAnnotateCondVarWait(const char *file, int line, const volatile void *cv, const volatile void *lock);

The WebKit prefix on these functions seems less than ideal given that they are in WTF.  A WTF prefix would be more in line with other straight C functions in WTF.

The functions should be updated to match the WebKit style guide: Abbreviations shouldn’t be used in the names, the * for pointers should be next to the type name, and argument names shouldn’t be abbreviated.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:80
> +#define ANNOTATE_HAPPENS_AFTER(obj) /* empty */

It’s not clear to me that the comments here add any value.

> Source/JavaScriptCore/wtf/ThreadSafeShared.h:65
> +#include <wtf/DynamicAnnotations.h>

Given that ThreadSafeShared.h is included by projects outside of JavaScriptCore (e.g., by WebCore) I suspect that this #include will break the Mac build unless you add a forwarding header.

> Source/JavaScriptCore/wtf/ThreadSafeShared.h:108
> +        // See http://code.google.com/p/data-race-test/wiki/DynamicAnnotations#Reference_counting

I can’t help but feel that if the macro names were clearer then we wouldn’t need comments like this where they’re used.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.