I want to claim this task.

Here coming the first rule. I was checking just for method names, not 
signatures,
because signatures for P/Invoke can be quite different, while method is 
referring to
the same external one.

Good point! Signatures often varies and many are buggy! 

I think it's enough to know the name and dll. One extra step would be to keep 
(and
check) the number (not type) of the parameters - but we risk skipping some 
really bad
signatures (which would be useful to convert into managed code ;-)

Second rule.

And the third one.

Docs:

UseManagedAlternativesToPInvokeRule

This rule warns the developer if certain external (P/Invoke) methods are being 
called
in case they have managed alternatives provided in .NET framework.

Bad example:
[DllImport("kernel32.dll")]
static extern void Sleep (uint dwMilliseconds);

// usage:
Sleep (U2000);


Good example:
System.Threading.Thread.Sleep (2000);


*********************************************************************

MarshalStringsInPInvokeDeclarationsRule

This rule warns the developer if the charset has not been specified for strings 
that
are passed as parameters to a P/Invoke method (this affects System.String and
System.Text.StringBuilder parameters).

Bad example:

[DllImport("coredll.dll")]
static extern int SHCreateShortcut (StringBuilder szShortcut, StringBuilder 
szTarget);

Good example:

[DllImport("coredll.dll", CharSet = CharSet.Auto)]
static extern int SHCreateShortcut (StringBuilder szShortcut, StringBuilder 
szTarget);

*********************************************************************

UseSTAThreadAttributeOnSWFEntryPoints

This rule checks executable assemblies that reference System.Windows.Forms to 
ensure
that their entry point is decorated with [System.STAThread] attribute and is NOT
decorated with [System.MTAThread] attribute, or otherwise Windows Forms may not 
work
properly.

Bad example #1 (no attributes):
public class WindowsFormsEntryPoint {
    static void Main ()
    {
    }
}

Bad example #2 (MTAThread)
public class WindowsFormsEntryPoint {
    [MTAThread]
    static void Main ()
    {
    }
}

Good example #1 (STAThread):
public class WindowsFormsEntryPoint {
    [STAThread]
    static void Main ()
    {
    }
}

Good example #2 (not Windows Forms):
public class ConsoleAppEntryPoint {
    static void Main ()
    {
    }
}

General note: please clean your using clauses, there are a lot of unrequired 
ones
(which slows down compilation). If you're (still) using VS.NET then there's an 
option
do remove unused ones.

UseManagedAlternativesToPInvokeRule
* Nice selection :)
* You can't use Environment.Version because that would always be > 2 since 
Gendarme's
compilation requires > 2. You must check the analyzed assembly runtime 
requirements
and get different lists (or better keep a minimal version with each entry). See 
the
IsGeneratedCode rock (in SVN) to see how it can be done.
* Don't repeat the "Use {0} method instead" since it will be hard to localize 
in the
future. Keeping the method name should be enough.
* The PInvokeCall.Module property seems unused (and should be removed)

MarshalStringsInPInvokeDeclarationsRule
* CharSet isn't enough. It's commonly used but sometimes you need to use a 
MarshalAs
attribute on a parameter (e.g. if a method was converting from 2 string types). 
That
needs to be added to the documentation (e.g. in the examples) too.
* The message could be better if you keep the parameters names (some p/invoke 
can
have *many* parameters). E.g. replace the bool hasStringParams with a string 
(init to
String.Empty) then add any "bad" parameters and check for .Length after the 
loop.
* It wasn't part of the task but please add a comment about structures. We 
should
enhance the rule, or have another one, to check for structures used in p/invoke 
calls
(because they can contain strings).

UseSTAThreadAttributeOnSWFEntryPointsRule
* The returned message can be confusing. Split it to explain why STAThread is 
needed
(missing) or why MTAThread should be replaced with STAThread.
* The NoAttributeMain class is not used on your tests. Could it be a left-over 
before
you added a SWF boolean to add the assembly references ? However it seems 
required
since test coverage doesn't include the return success if no SWF is referenced.

uho, last entry was mine ;-)

> You can't use Environment.Version because that would always be > 2 since 
Gendarme's
compilation requires > 2.
> Don't repeat the "Use {0} method instead" 

Sure, fixed.

> The PInvokeCall.Module property seems unused (and should be removed)
Nope, it isn't! It is used when searching for alternatives (all fields are 
compared
in value type Equals () implementation, and module too - though I could of 
course
override Equals to make it work faster - and I will).

> It's commonly used but sometimes you need to use a MarshalAs attribute on a 
parameter
So what? If MarshalAs is specified but charset isn't, should it fire? I don't 
quite
understand this bit so it remains not implemented yet.

> The message could be better if you keep the parameters names

Fixed.

> add a comment about structures

OK.

> The returned message can be confusing. Split it
> The NoAttributeMain class is not used on your tests.

Fixed.

Updated UseManagedAlternativesToPInvokeRule (+ Equals () and minor changes).

> If MarshalAs is specified but charset isn't, should it fire?

If all string/StringBuilder parameters are covered by MarshalAs attribute then
there's no need for CharSet.

I'm starting reviewing the other tasks right away

UseSTAThreadAttributeOnSWFEntryPointsRule
* I found out why the coverage was so low, even if a lot of tests are present:
(1) assembly.Kind != AssemblyKind.Windows
While this is correct (in the sense that it should be that way) the rule must 
not
check for this (there's already another rule to report that). Adding this check 
can
hide the real problem that the rule checks for (e.g. in case the console window 
was
required by the SWF app).
(2) there's no test case if no attributes is present (one can be made easily 
reusing
TestNoTANonSWFAssembly) and if there's no entry point (once can be made easily 
by
using the "assembly" field which represent the unit test assembly).
With both changes I get 100% coverage :)

You made a few typos in your last changes
* if (!hasSTA && hasSTA)
* text = "In order for Windows Forms to work properly, place [System.MTAThread]
attribute upon the entry point."

since these are minor, and already fixed in my local repo, there's no need to 
update
your code - I'll commit this soon to SVN.

UseManagedAlternativesToPInvokeRule
>> The PInvokeCall.Module property seems unused (and should be removed)
> Nope, it isn't!

I meant the property, not the field. Even in your last version no code use the 
getter
or setter (and a few other things as well, which I'll remove before committing).

Note: I'm removing BitBlt since it's often used with raster mode not possible 
with
GDI+ (nor it's System.Drawing wrapper).

So, what am I supposed to do to close the task?

> So, what am I supposed to do to close the task?
See comment #11.

If CharSet isn't present, then iterate all parameters for string/StringBuilder 
and
check if they each have a MarshalAs attribute. Add test case for this (good and 
bad)
and it should be complete.

I'll be away for several days so I'll post it later.
Please upload your updated version of file for me to change (since you've made 
some
modifications).

I have not modified the file.
Moving date to last full day for completing a GHOP task.

See ya later!
Sebastien

Now that I've finished some school stuff, I'm at last able to complete the task 
:-)

Well you did more than I expected by handling structures :) even if structs are
handled partially (see last note).

* Method HasCharsetSpecified doesn't work because [StructLayout] is a
pseudo-attribute, i.e it's not a *custom* attribute and, as such, is not part 
of the
CustomAttributeCollection.

* Check structure.Attributes for SequentialLayout (or the helper 
IsSequentialLayout
in newer Cecil versions) or ExplicitLayout (but not the default AutoLayout). 
Next
check for Is[Auto|ANsi|Unicode]Class to see if CharSet is specified.

* Please add more unit tests for those (since the current one didn't catch the
problem ;-)

* some debugging stuff is left ;-)
...
            if (messages != null)
                foreach (Message m in messages)
                    Console.WriteLine (m.Text);
...

* about the structures, I'm not sure it's totally correct since 
    - right now it miss structures that aren't used for p/invoke inside the analyzed
assembly
    - in the near-future (AssemblyResolver) it will report problem outside the
analyzed assembly
Both are minor problems outside the task description but please add a comment 
in the
rule for both cases.

Thanks!

OK, I'll write you back this evening (morning for you :-).

Updated version (structure handling is disabled).

MarshalStringsInPInvokeDeclarationsRule (update)

This rule warns the developer if the charset has not been specified for strings 
that
are passed as parameters to a P/Invoke method if they aren't decorated with
[MarshalAs] attribute (this affects System.String and
System.Text.StringBuilder parameters).

Bad example:

[DllImport("coredll.dll")]
static extern int SHCreateShortcut (StringBuilder szShortcut, StringBuilder 
szTarget);

Good example:

[DllImport("coredll.dll", CharSet = CharSet.Auto)]
static extern int SHCreateShortcut (StringBuilder szShortcut, StringBuilder 
szTarget);


Good example #2:
[DllImport("coredll.dll")]
static extern int SHCreateShortcut ([MarshalAs(UnmanagedType.LPTStr)] 
StringBuilder
szShortcut, [MarshalAs(UnmanagedType.LPTStr)] StringBuilder szTarget);

Everything is fine, closing this task.
Congratulation for your GHOP participation!
I hope we will see you soon again :)
Sebastien