Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Message-ID: <452D302C.2070303@roguewave.com>
Date: Wed, 11 Oct 2006 11:55:56 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.13) Gecko/20060417
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [PATCH] stdcxx target warning counting
References: <452BB8CB.1050201@roguewave.com>
In-Reply-To: <452BB8CB.1050201@roguewave.com>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Andrew Black wrote:
> Greetings all.
> 
[...]
> +    "  --warn modes:\n"
> +    "    acc  HP aCC\n"
> +    "    eccp EDG\n"

This should be "eccp EDG eccp," not just EDG (that's the name
of the company that makes the eccp front end).

> +    "    gcc  GNU gcc\n"
> +    "    icc  Intel icc\n"

Does this handle the Windows version of the compiler or just
the Linux one? If both, we should change the text to Intel C++
to make it clear (or add an alias for icl).

> +    "    mips SGI MIPSpro (IRIX)\n"

I would change this to "mipspro SGI MIPSpro" (no need for the
IRIX bit),

> +    "    osf  Compaq C++ for Tru64\n"

this to "cxx Compaq C++" (OSF refers to the OS and we probably
don't need the Tru64 part),

> +    "    spro Sun Forte/Studio\n"

this probably to "sunpro Sun C++" (since we have sunpro.config),

> +    "    vac  IBM Visual Age\n"

and this to "xlc  IBM XLC++."

You could also add aliases if you like (e.g., for IBM VisualAge,
we could have "vacpp IBM VisualAge C++" in addition to XLC++).

> @@ -365,7 +385,76 @@
>      return 0;
>  }
>  
> +/**
> +   Helper function to parse a warning value string
>  
> +   @param opts ulimit value string to pares
> +   @see child_limits
> +*/
> +static bool
> +parse_warn_opts (const char* opts, struct target_opts* defaults)
> +{
> +    static const struct {
> +        const char* name;
> +        const char* caps;
> +        const char* mixd;
> +        size_t len;
> +        const char* pat;
> +    } warn_set [] = {
> +        { "acc", "ACC", "Acc", 3, "Warning " },

We could probably just compare the names without regard to case,
don't you think? Or did you have a specific reason for recognizing
only these three forms?

[...]
> Index: util/output.cpp
> ===================================================================
> --- util/output.cpp	(revision 454401)
> +++ util/output.cpp	(working copy)
> @@ -109,7 +109,13 @@
>                      return;
>                  }
>              }
> -            /* else if (1 < r_lvl) warning*/
> +            else if (1 < r_lvl) {
> +                status->t_warn += r_active;
> +                if (status->t_warn < r_active) {
> +                    status->status = ST_OVERFLOW;

Is this arithmetic overflow? (How long would it take for the counter
to reach 4 billion and how big would the input file have to be in
order for it to happen? I suspect we can drop this check and keep
it simple.)

[...]

> Index: util/display.cpp
> ===================================================================
> --- util/display.cpp	(revision 454401)
> +++ util/display.cpp	(working copy)
> @@ -40,8 +40,8 @@
>  */
>  static void print_header_plain ()
>  {
> -    puts ("NAME                      STATUS ASSERTS FAILED PERCNT    USER     "
> -          "SYS    WALL");
> +    puts ("NAME                      STATUS WARN ASSERTS FAILED PERCNT    "
> +          "USER     SYS    WALL");

I would suggest to rename WALL to REAL as that's what the times
command reports (and it's probably more a well known term than
WALL alone).

[...]
> Index: util/runall.cpp
> ===================================================================
> --- util/runall.cpp	(revision 454401)
> +++ util/runall.cpp	(working copy)
[...]
> +static
> +void count_warnings (const char* const target, unsigned* counter, 
> +                     const char* const match)
> +{
> +    size_t path_len;
> +    char* tmp_name;
> +    FILE* data;
> +
> +    assert (0 != target);
> +    assert (0 != counter);
> +
> +    if (0 == match)
> +        return; /* Don't have a match string, so don't tally warnings */
> +
> +    path_len = strlen (target);
> +    tmp_name = (char*)RW_MALLOC (path_len + 5);

I don't see a matching free() for this allocation.

[...]
> @@ -212,10 +282,41 @@
>  {
>      struct stat file_info;
>      int exists = 1;
> +    size_t path_len;
> +    char* tmp_name;
>  
>      assert (0 != target);
>      assert (0 != status);
>  
> +    path_len = strlen (target);
> +
> +#if !defined (_WIN32) && !defined (_WIN64)

As I mentioned before, these conditionals can be simplified to
just #ifndef _WIN32 since _WIN32 is guaranteed to be defined in
64-bit mode.

> +    /* Otherwise, check for the .o file on non-windows systems */
> +    tmp_name = (char*)RW_MALLOC (path_len + 3);

Is there a call to free() corresponding to this malloc()? (I
don't see it in the patch but it might be in the rest of the
code).

Martin