Closed GoogleCodeExporter closed 9 years ago
I want to claim this task.
Original comment by dan.abra...@gmail.com
on 19 Jan 2008 at 7:00
Original comment by sebastie...@gmail.com
on 19 Jan 2008 at 7:08
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.
Original comment by dan.abra...@gmail.com
on 20 Jan 2008 at 5:06
Attachments:
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 ;-)
Original comment by sebastie...@gmail.com
on 20 Jan 2008 at 5:13
Second rule.
Original comment by dan.abra...@gmail.com
on 20 Jan 2008 at 8:55
Attachments:
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 ()
{
}
}
Original comment by dan.abra...@gmail.com
on 20 Jan 2008 at 11:42
Attachments:
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.
Original comment by sracque...@gmail.com
on 21 Jan 2008 at 1:29
uho, last entry was mine ;-)
Original comment by sebastie...@gmail.com
on 21 Jan 2008 at 1:39
> 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.
Original comment by dan.abra...@gmail.com
on 21 Jan 2008 at 3:11
Attachments:
Updated UseManagedAlternativesToPInvokeRule (+ Equals () and minor changes).
Original comment by dan.abra...@gmail.com
on 21 Jan 2008 at 3:23
Attachments:
> 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
Original comment by sebastie...@gmail.com
on 21 Jan 2008 at 1:44
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.
Original comment by sebastie...@gmail.com
on 21 Jan 2008 at 2:41
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).
Original comment by sebastie...@gmail.com
on 21 Jan 2008 at 3:45
So, what am I supposed to do to close the task?
Original comment by dan.abra...@gmail.com
on 21 Jan 2008 at 9:16
> 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.
Original comment by sebastie...@gmail.com
on 21 Jan 2008 at 9:20
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).
Original comment by dan.abra...@gmail.com
on 22 Jan 2008 at 4:36
I have not modified the file.
Moving date to last full day for completing a GHOP task.
See ya later!
Sebastien
Original comment by sebastie...@gmail.com
on 22 Jan 2008 at 1:13
Now that I've finished some school stuff, I'm at last able to complete the task
:-)
Original comment by dan.abra...@gmail.com
on 28 Jan 2008 at 4:36
Attachments:
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!
Original comment by sebastie...@gmail.com
on 28 Jan 2008 at 10:31
OK, I'll write you back this evening (morning for you :-).
Original comment by dan.abra...@gmail.com
on 29 Jan 2008 at 5:08
Updated version (structure handling is disabled).
Original comment by dan.abra...@gmail.com
on 29 Jan 2008 at 7:45
Attachments:
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);
Original comment by dan.abra...@gmail.com
on 29 Jan 2008 at 8:03
Everything is fine, closing this task.
Congratulation for your GHOP participation!
I hope we will see you soon again :)
Sebastien
Original comment by sebastie...@gmail.com
on 30 Jan 2008 at 12:58
Original issue reported on code.google.com by
sebastie...@gmail.com
on 16 Jan 2008 at 3:36