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

Added regex logic to handle errors and warnings displayed on console #51

Closed
wants to merge 19 commits into from
Closed

Conversation

David-Maisonave
Copy link
Contributor

Added regex logic to handle errors and warnings displayed on console.

  1. Handles jump-to file, line no, and inline position.
  2. Handles console coloring for errors

Added regex logic to handle errors and warnings displayed on console.
1. Handles jump-to file, line no, and inline position.
2. Handles console coloring for errors
@@ -422,6 +422,69 @@ bool CWarningAnalyzer::match( const TCHAR* str )
postr4 = skip_tabspaces(postr4);
m_nChar = _ttoi(postr4);
}
else // *** START: New regex Warning/Error parser
{ // *** All of the new regex Warning/Error parser is within this else block ***
std::wsmatch match;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use std::match_results<const TCHAR*> instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

else // *** START: New regex Warning/Error parser
{ // *** All of the new regex Warning/Error parser is within this else block ***
std::wsmatch match;
const std::wstring HeyStack = str;
Copy link
Owner

@d0vgan d0vgan Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use std::basic_string<TCHAR> instead.
This applies to all the further occurrences of std::wstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

static std::map<int,int> ErrPositionIndicator;
static std::wstring PreviousFileName;
static int PreviousLineNo = 0;
std::wregex std_Needle = std::wregex( L"(?:(([a-zA-Z]:[\\\\/]|\\.)[^.]*\\.[^:\"\\(\\s]{1,8})|(^\\w[^\\s:\\\\/]*\\.\\w{1,8}))(?=[\\s:(\",oline]{1,9}[0-9]+)" ); // Regex to find file name
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method CWarningAnalyzer::match is called for every line that is printed to NppExec's Console, so it may be too expensive to create the same instance of std_Needle again and again. Let's rather make the std_Needle to be a member variable that is created just once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

wcscpy_s( m_FileName, sizeof( m_FileName )/sizeof( m_FileName[0]), filename.c_str() );
m_nLastFoundIndex = 0;
*m_Filter = TFilter();
std_Needle = std::wregex( L"(?:^[^0-9a-zA-Z_]+|.*line )([0-9]+).*" );// Regex to find file number with file name preceeding it
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rather make it an additional member variable that is created just once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

*(COLORREF*)(&m_Filter->Effect.Red) = COLOR_CON_TEXTERR; //Only color if found file name and line number
m_nLine = std::stoi( strLineNo.c_str() );
PreviousLineNo = m_nLine;
std_Needle = std::wregex( L".*[^a-zA-Z](?:[0-9][,:]\\s*|[0-9] char[,:])([0-9]+).*" ); // Regex to find nChar with file number preceeding it
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rather make it an additional member variable that is created just once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

else
{
Suffix = match.suffix();
std_Needle = std::wregex( L"\\s+[\\x5E~]" ); // Regex to find error possition indicator
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rather make it an additional member variable that is created just once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

}
else
{
std_Needle = std::wregex( L"^[\\.\\s]+[\\x5E~]" ); // Regex to find error possition indicator starting at the begging of a line
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rather make it an additional member variable that is created just once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. done.

std::wstring strLineNo = std::regex_replace( Suffix, std_Needle, L"$1" ); // Replace to get file number only
if ( strLineNo.size() && isdigit( strLineNo[0] ) )
{
*(COLORREF*)(&m_Filter->Effect.Red) = COLOR_CON_TEXTERR; //Only color if found file name and line number
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks somewhat unsafe, no? (I mean, we assume that there is an implicit unsigned char dummy right after unsigned char Blue within the TEffect structure to match the sizeof(COLORREF) and that these members are aligned by exactly 1 byte.). I'd prefer explicit setting of the Effect.Red, Effect.Green and Effect.Blue members.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed TEffect so that the three unsigned char variables are in a union with COLORREF variable (rgb). And I changed line 449 to the following:
m_Filter->Effect.rgb = COLOR_CON_TEXTERR;
That allows RGB macro to safely be used to set the colors. It's common programming practice on multiple languages to use RGB for color settings. And it's easy to find RGB color charts online.
https://www.ginifab.com/feeds/pms/color_picker_from_image.php
https://www.w3schools.com/colors/colors_converter.asp
http://www.zonums.com/online/color_converter/

m_Filter->Effect.Bold = true;
m_Filter->Effect.Enable = true;
std::transform( filename.begin(), filename.end(), filename.begin(), ::tolower );
if ( filename.size() > 4 && filename.substr( filename.size() - 4 ) == L".exe" )
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may use lstrcmp to compare the existing last 4 characters of filename with _T(".exe") instead of using a temporary string produced by substr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. When I went to make the changed, I realized I could just use lstrcmpi, and removed the previous line that changed it to lowercase.

std_Needle = std::wregex( L"^[\\.\\s]+[\\x5E~]" ); // Regex to find error possition indicator starting at the begging of a line
if ( std::regex_search( HeyStack, match, std_Needle ) ) // Find error possition indicator
{
std::wstring ErrorPositionIndicator = match[0].str().size() > 2 && match[0].str().substr( 0, 3 ) == L"..." ? match[0].str().substr( 3 ) : match[0].str();
Copy link
Owner

@d0vgan d0vgan Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use
match[0].str().compare( 0, 3, _T("...") ) == 0
instead of
match[0].str().substr( 0, 3 ) == L"..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Although the difference is compiled away.

// ToDo: ********** If needed, append your new *[Run Handler]* HERE by adding another "else if" block to handle your file extension type. **********
// --------------------------------------------------------------------------------------
// Example #1 *[Run Handler]* for files with extension .foo, where MyFooScriptRunner.exe is an executable that knows what to do with (.foo) files.
else if $(ext) == .foo
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else if is not a part of a comment, so let's unindent it (remove the leading spaces) to avoid visual confusion of its scope.
This comment applies to all the other indented else if as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still working on DebugAnyThing.NppExec.
Is there a way to select the files that should be included in the pull request?
The only files I intended to include was WarningAnalyzer.cpp, WarningAnalyzer.h, NppExecEngine.cpp, and DlgConsole.cpp.

WarningAnalyzer.cpp and WarningAnalyzer.h are the only ones with the regex code changes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may create a separate git branch for each independent change (e.g. one branch for the regex to handle errors, another branch for the NppExec script, etc.).
In your case, when everything is already in your "master" branch, probably the best way to separate different things is to:

  1. cancel this pull request
  2. manually copy your changed files to some temporary folder
  3. reset all your changes in the "master" branch
  4. create a new branch e.g. "feature/regex-warn-analyzer", checkout that new branch and manually copy the related changed files from your temporary folder (created in the step 1 above) to this new branch
  5. create a new pull request against this new branch (it will replace the previously cancelled pull request)
  6. checkout the "master" branch and create a new branch e.g. "feature/nppexec-scripts", checkout that new branch and manually copy the related changed files from your temporary folder to this new branch. Thus you'll have two independent branches with independent changes
  7. when all the changes in the "feature/nppexec-scripts" branch are ready, create a pull request against this branch.

@@ -188,6 +188,9 @@
<ImportLibrary>$(OutDir)NppExec.lib</ImportLibrary>
<AdditionalDependencies>shlwapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
</Link>
<PostBuildEvent>
<Command>copy "$(TargetPath)" "%APPDATA%\Notepad++\plugins\$(TargetName)"</Command>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I use a portable version of Notepad++, thus there is no "%APPDATA%\Notepad++\plugins$(TargetName)" on my machine.
Also, there is no "%APPDATA%\Notepad++\plugins$(TargetName)" on a generic build machine where NppExec's sources are built.
Maybe let's customize this command to copy only in case of the folder "%APPDATA%\Notepad++\plugins" exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually didn't intend to merge NppExec_VC2017.vcxproj change with the master. I pushed it on my fork so I wouldn't loose the change.
I did add the if exist before the copy command, so if you want, you could add it to the master.

{
if ( std::regex_search( HeyStack, match, m_rgxFindErrPosIndicatorAtStartOfLine ) ) // Find error possition indicator
{
tstring ErrorPositionIndicator = match[0].str().size() > 2 && match[0].str().compare( 0, 3, _T("...") ) == 0 ? match[0].str().substr( 3 ) : match[0].str();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's enclose the condition in parentheses? Frankly, I needed several attempts to understand how this long construction with && and ? works :)

PreviousFileName = filename;
ErrPositionIndicator.clear();
}
wcscpy_s( m_FileName, sizeof( m_FileName )/sizeof( m_FileName[0]), filename.c_str() );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use lstrcpy instead of wcscpy_s.

@David-Maisonave
Copy link
Contributor Author

Closing this pull request because it included more files then intended.
Moved the desired changes to #55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants